On 5/6/21 4:47 PM, Stefan Hajnoczi wrote:
> On Thu, May 06, 2021 at 11:01:47AM +0200, Philippe Mathieu-Daudé wrote:
>> On 5/6/21 10:53 AM, Stefan Hajnoczi wrote:
>>> On Wed, May 05, 2021 at 11:10:30PM +0200, Philippe Mathieu-Daudé wrote:
>>>> Use autofree heap allocation instead of variable-length
>>>> array on the stack.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
>>>> ---
>>>> hw/block/dataplane/virtio-blk.c | 7 ++++---
>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> Why?
>>
>> The motivation behind removing all variable-length allocations
>> (and adding CPPFLAG+=-Wvla at the end) is to avoid security
>> vulnerabilities such CVE-2021-3527.
>
> I see. Please mention it in the commit description. There could be other
> reasons for this change, like minimizing stack usage, so I wasn't sure
> why.
Fair enough.
>>> This is a performance-critical code path and BITS_TO_LONGS(nvqs) is
>>> small.
>>
>> OK, having looked better at nvqs, I suppose this is preferred:
>>
>> -- >8 --
>> @@ -60,7 +60,7 @@ static void notify_guest_bh(void *opaque)
>> {
>> VirtIOBlockDataPlane *s = opaque;
>> unsigned nvqs = s->conf->num_queues;
>> - unsigned long bitmap[BITS_TO_LONGS(nvqs)];
>> + unsigned long bitmap[BITS_TO_LONGS(VIRTIO_QUEUE_MAX)];
>> unsigned j;
>>
>> memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
>> ---
>>
>> Would that work for you?
>
> It's a little risky since s->batch_notify_vqs does not have
> sizeof(bitmap). That makes uninitialized data and buffer overflows more
> likely. Your example has the bug:
>
> memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
> ^^^^^^^^^^^^^^
> Accesses beyond the end of s->batch_notify_vqs[].
Doh I missed that :/
> Can we eliminate bitmap[] entirely by using bitops.h APIs on
> s->batch_notify_vqs instead?
You meant the bitmap.h API I assume, OK, better idea!
Thanks,
Phil.
