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

[Kevin Wolf]

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.

> > On 11/16/2018 3:31 AM, Paolo Bonzini wrote:
> >> Because the CMB BAR has a min_access_size of 2, if you read the last
> >> byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one
> >> error.  This is CVE-2018-16847.
> >>
> >> Another way to fix this might be to register the CMB as a RAM memory
> >> region, which would also be more efficient.  However, that might be a
> >> change for big-endian machines; I didn't think this through and I don't
> >> know how real hardware works.  Add a basic testcase for the CMB in case
> >> somebody does this change later on.
> >>
> >> Cc: Keith Busch <address@hidden>
> >> Cc: address@hidden
> >> Cc: Li Qiang <address@hidden>
> >> Signed-off-by: Paolo Bonzini <address@hidden>
> >> ---
> >>   hw/block/nvme.c        |  2 +-
> >>   tests/Makefile.include |  2 +-
> >>   tests/nvme-test.c      | 58 +++++++++++++++++++++++++++++++++++++++---
> >>   3 files changed, 57 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> >> index 09d7c90259..5d92794ef7 100644
> >> --- a/hw/block/nvme.c
> >> +++ b/hw/block/nvme.c
> >> @@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
> >>       .write = nvme_cmb_write,
> >>       .endianness = DEVICE_LITTLE_ENDIAN,
> >>       .impl = {
> >> -        .min_access_size = 2,
> >> +        .min_access_size = 1,
> >>           .max_access_size = 8,
> >>       },
> >>   };

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.

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?

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.

Hm... But memory_region_access_valid() doesn't even check min and max
access size if an accepts function pointer isn't given as well. Yet,
there are devices that set min/max, but not accepts. Am I missing where
this actually takes effect or are they buggy?

This stuff really confuses me.

Kevin