Re: [Qemu-devel] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB

[Paolo Bonzini]

On 19/11/18 18:43, Kevin Wolf wrote:
> Am 19.11.2018 um 18:09 hat Paolo Bonzini geschrieben:
>> On 19/11/18 16:23, Mark Kanda wrote:
>>> For CVE-2018-16847, I just noticed Kevin pulled in Li's previous fix (as
>>> opposed to this one). Was this done in error?
>>
>> Probably.  Kevin, can you revert and apply this one instead?  I don't
>> care if 3.1 or 3.2, but the previous fix is pointless complication.
> 
> I was waiting for you to address Li Qiang's review comments before I
> apply it. I can revert the other one once this is ready.

Sorry, I forgot to send it.  Did it now.

> Anyway, that .min_access_size influences the accessible range feels
> weird to me. Is this really how it is meant to work? I expected this
> only to influence the allowed granularity of accesses, and that the
> maximum accessible offset of the memory region is size - access_size.
>> Does this mean that the size parameter of memory_region_init_io() really
> means we allow access to offsets from 0 to size + impl.min_access_size - 1?
> If so, this is very surprising and I wonder if this is really the only
> device that gets it wrong.

Usually the offset is a register, so an invalid value will simply be
ignored by the device or reported as a guest error.

> For nvme it doesn't matter much because it can trivially support
> single-byte accesses, so this change is correct and fixes the problem,
> but isn't the real bug in access_with_adjusted_size(), which should
> adjust the accessed range in a way that it doesn't exceed the size of
> the memory region?

Hmm, what's happening is complicated.  memory_access_size is clamping
the access size to 1 because impl.unaligned is false.  However,
access_with_adjusted_size is bringing it back to 2 because it does

    access_size = MAX(MIN(size, access_size_max), access_size_min);

So we could do something like

diff --git a/exec.c b/exec.c
index bb6170dbff..f1437b2be6 100644
--- a/exec.c
+++ b/exec.c
@@ -3175,7 +3175,11 @@
     if (!mr->ops->impl.unaligned) {
         unsigned align_size_max = addr & -addr;
         if (align_size_max != 0 && align_size_max < access_size_max) {
-            access_size_max = align_size_max;
+            unsigned access_size_min = mr->ops->valid.min_access_size;
+            if (access_size_min == 0) {
+                access_size_min = 1;
+            }
+            access_size_max = MAX(min_access_size, align_size_max);
         }
     }

Then I think the access size would remain 2 and and
memory_region_access_valid would reject it as unaligned.  That would
avoid the bug, but then nvme should be setting valid.min_access_size and
the exec.c patch alone would not be enough.

> I'm not sure why impl.min_access_size was set to 2 in the first place,
> but was valid.min_access_size meant maybe? Though if I read the spec
> correctly, that one should be 4, not 2.

I don't see any requirement for the CMB (section 4.7 in my copy)?

Paolo