Re: [PULL 11/13] virtio-gpu: Handle resource blob commands

["Alex Bennée" <alex.bennee@linaro.org>]

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> On 11/1/24 18:35, Peter Maydell wrote:
>> On Tue, 29 Oct 2024 at 12:11, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>> From: Robert Beckett <bob.beckett@collabora.com>
>>>
>>> Support BLOB resources creation, mapping, unmapping and set-scanout by
>>> calling the new stable virglrenderer 0.10 interface. Only enabled when
>>> available and via the blob config. E.g. -device virtio-vga-gl,blob=true
>>>
>>> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> # added set_scanout_blob
>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> Message-Id: <20241024210311.118220-12-dmitry.osipenko@collabora.com>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> Hi; Coverity points out some possible issues with this commit:
>> 
>> 
>>> +    fb.bytes_pp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(fb.format), 8);
>>> +    fb.width = ss.width;
>>> +    fb.height = ss.height;
>>> +    fb.stride = ss.strides[0];
>>> +    fb.offset = ss.offsets[0] + ss.r.x * fb.bytes_pp + ss.r.y * fb.stride;
>>> +
>>> +    fbend = fb.offset;
>>> +    fbend += fb.stride * (ss.r.height - 1);
>>> +    fbend += fb.bytes_pp * ss.r.width;
>> 
>> Here 'fbend' is a uint64_t, but fb.stride, fb.bytes_pp,
>> ss.r.height and ss.r.width are all uint32_t. So these
>> multiplications will be done as 32x32->32 before being
>> added to fbend, potentially overflowing. This probably
>> isn't what was intended.
>> 
>> (This is Coverity CID 1564769, 1564770.)
>> 
>> The calculation of fb.offset also might overflow, but
>> Coverity doesn't spot that because fb.offset is uint32_t
>> and so the whole calculation is 32-bit all the way through
>> without a late-32-to-64 extension.
>
> Will make patch to silence this Coverity warning. AFAICT, there are
> other ways to cause integer overflows in that code, though I assume it
> all should be harmless. Thanks!

Ahh I'm happy for you to do it as you are more familiar with the code.
Note the duplicated code.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro