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
