bhyve nvme: Fix out-of-bound IOV array access

[chuck]

Summary

NVMe operations indicate the memory region(s) associated with a command
via physical region pages (PRPs). Since each PRP has a fixed size,
contiguous memory regions larger than the PRP size require multiple PRP
entries.

Instead of issuing a blockif call for each PRP, the NVMe emulation
concatenates multiple contiguous PRP entries into a single blockif
request. The test for contiguous regions has a bug such that it
mistakenly treats an initial address of zero as a contiguous range and
concatenates it with the previous. But because there is no previous IOV,
the concatenation code corrupts the IO request structure and leads to a
segmentation fault when the blockif request completes.

Fix is to test for the existence of a previous range before trying to
concatenate the current range with the previous one.

PR: 264177
Reported by: Robert Morris <[e-mail]>
MFC after: 2 weeks

[gusev.vitaliy_gmail.com]

1. I think patch is good, however I have questions (not directly related to the patch) for lba naming. LBA - logical block address - it should be enumerated with blocks, i.e. 1,.... However pci_nvme_append_iov_req() is called from nvme_write_read_blockif() and offset is passed as lba.

Do you think it is worth renaming lba to offset?

2. And one more suggestion, what if code will be:

	if (req->io_req.br_iovcnt > 0 && (req->prev_gpaddr + req->prev_size) == gpaddr) {

3. Question related to description.

 The test for contiguous regions has a bug such that it
mistakenly treats an initial address of zero as a contiguous range and
concatenates it with the previous. But because there is no previous IOV,
the concatenation code corrupts the IO request structure and leads to a
segmentation fault when the blockif request completes.

As I understood the first call of pci_nvme_append_iov_req() is always with req->io_req.br_iovcnt == 0, so this concatenation should be involved only for the subsequent calls of pci_nvme_append_iov_req() and it would good to point to it.

[chuck]

In D35328#800851, @gusev.vitaliy_gmail.com wrote:

1. I think patch is good, however I have questions (not directly related to the patch) for lba naming. LBA - logical block address - it should be enumerated with blocks, i.e. 1,.... However pci_nvme_append_iov_req() is called from nvme_write_read_blockif() and offset is passed as lba.

Do you think it is worth renaming lba to offset?

Agreed, that is a change worth making

2. And one more suggestion, what if code will be:

	if (req->io_req.br_iovcnt > 0 && (req->prev_gpaddr + req->prev_size) == gpaddr) {

What you posted is very close to the original change I made. My hope was the intent of this version was clearer and compiled down to roughly the same assembly code.

3. Question related to description.

 The test for contiguous regions has a bug such that it
mistakenly treats an initial address of zero as a contiguous range and
concatenates it with the previous. But because there is no previous IOV,
the concatenation code corrupts the IO request structure and leads to a
segmentation fault when the blockif request completes.

As I understood the first call of pci_nvme_append_iov_req() is always with req->io_req.br_iovcnt == 0, so this concatenation should be involved only for the subsequent calls of pci_nvme_append_iov_req() and it would good to point to it.

I had hoped the last sentence covered this, but it sounds like it missed. Is

Fix is to always skip concatenation for the initial address range.

any better? If not, I'm open to rewording :)

[chuck]

Implement suggested rename of variable

[jhb]

I had read 'initial address of zero' as meaning that the first iovec had iov_base of 0. Saying something like "the first vector" probably is a bit less ambiguous.

[gusev.vitaliy_gmail.com]

In D35328#800878, @chuck wrote:

What you posted is very close to the original change I made. My hope was the intent of this version was clearer and compiled down to roughly the same assembly code.

Yes, it has the same result. However splitting condition with range_is_contiguous makes perception that case with br_iovcnt == 0 is something different and is not related, whereas using

(req->io_req.br_iovcnt > 0 && (req->prev_gpaddr + req->prev_size) == gpaddr)

produces right understanding the second sub-condition is valid only then first sub-condition is right.

However to make it more clear:

if (req->prev_size != 0 && (req->prev_gpaddr + req->prev_size) == gpaddr)

It means if prev_size is zero, that hasn't been used yet.

[chuck]

In D35328#801068, @jhb wrote:

I had read 'initial address of zero' as meaning that the first iovec had iov_base of 0. Saying something like "the first vector" probably is a bit less ambiguous.

No, you read it correctly. When the iov_base is 0, it erroneously matches the previous address (0) and size (also 0).

[andy_omniosce.org]

This change looks good to me. Is there anything stopping this being merged?
(I'm doing another sync to illumos at the end of the month and I'd like to include this if possible)

[gusev.vitaliy_gmail.com]

In D35328#811976, @andy_omniosce.org wrote:

This change looks good to me. Is there anything stopping this being merged?
(I'm doing another sync to illumos at the end of the month and I'd like to include this if possible)

I would remove "range_is_contiguous" usage and unite all in one condition as "contiguous" case is only if both conditions are true:

(req->prev_size != 0 && (req->prev_gpaddr + req->prev_size) == gpaddr)

[chuck]

In D35328#811976, @andy_omniosce.org wrote:

This change looks good to me. Is there anything stopping this being merged?
(I'm doing another sync to illumos at the end of the month and I'd like to include this if possible)

There are a batch of these NVMe changes in the queue, but my testing has tripped over a failure causing a bhyve core dump. I don't believe the changes are the cause, but they seem to exacerbate whatever this issue is.

I'll try to focus on the bug to get this change into your sync window.

[chuck]

! In D35328#811978, @gusev.vitaliy_gmail.com wrote:

I would remove "range_is_contiguous" usage and unite all in one condition as "contiguous" case is only if both conditions are true:

(req->prev_size != 0 && (req->prev_gpaddr + req->prev_size) == gpaddr)

We have a difference in opinion here that I'm not sure how to reconcile. The checks are equivalent, but I feel a) the additional variable name makes the intent obvious and b) splitting the check makes the condition that caused the bug stand out. Your suggestion is more concise, but does it have other advantages perhaps I'm missing?