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 <keith.bu...@intel.com>
> >> Cc: qemu-block@nongnu.org
> >> Cc: Li Qiang <liq...@gmail.com>
> >> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> >> ---
> >> 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
