On 30. 07. 20, 8:46, Jiri Slaby wrote:
> Hi, OTOH, you should have CCed all the (public) lists.
>
> On 30. 07. 20, 4:50, 张云海 wrote:
>> Zhang Xiao points out that the check should use > instead of >=,
>> otherwise the last line will be skip.
>> I agree with that, so I modify the patch.
>> Could you please verify that it is still correct and sufficient?
>
> IMO, yes, correct -- I was thinking about this yesterday too. Just an
> example: hypothetically, if we had:
> size_row = 1
> tail = 29
> size = 30
>
> data[29] would be the last accessible member. Writing to data + tail (as
> "29 + 1 > 30" doesn't hold, so the modified check would pass), i.e.
> data[29] is still OK. So yes, > is OK, >= would waste space and would be
> actually incorrect.
>
>> BTW, Zhang Xiao also points out that the check after the memcpy can be
>> remove.
>> I also think that was right, but vgacon_scrollback_cur->tail may keep
>> the value vgacon_scrollback_cur->size in some case. That is not a
>> problem in vgacon_scrollback_update because of the check before the
>> memcpy. However, that may break some other code which assumes that
>> vgacon_scrollback_cur->tail won't be vgacon_scrollback_cur->size. I do
>> not know if there are such code, and if it is the code actually should
>> check it too. But I still not remove the check in the patch to make sure
>> it won't breaks other code.
>
> As I wrote about this yesterday:
> ===
> I am also not sure the test I was pointing out on the top of this
> message would be of any use after the change. But maybe leave the code
> rest in peace.
> ===
>
> I would let it as is in this particular code. Especially because
> vgacon_scrolldelta takes ->tail into consideration and I was too lazy to
> study the code there. But if you are willing to study the code there and
> confirm the check is superfluous, feel free to remove it. Perhaps in a
> separate patch. I was actually testing with the check removed and didn't
> hit any issue (which means, in fact, exactly nothing).
>
>> From ad143ede24ff4e61292cc9c96000100aacd97259 Mon Sep 17 00:00:00 2001
>> From: Yunhai Zhang <zhangyunhai@xxxxxxxxxxx>
>> Date: Tue, 28 Jul 2020 09:58:03 +0800
>> Subject: [PATCH] Fix for missing check in vgacon scrollback handling
>>
>> vgacon_scrollback_update() always left enbough room in the scrollback
>
> "leaves enough"
>
>> buffer for the next call, but if the console size changed that room
>> might not actually be enough, and so we need to re-check.
>
> Also, could you add reasoning why you are adding the check to the loop
> and not outside (for instance, use your reasoning with numbers or CSI M
> as an example).
>
> Could you add a sample output here, something like I had:
> ===
> This leads to random crashes or KASAN reports like:
> BUG: KASAN: slab-out-of-bounds in vgacon_scroll+0x57a/0x8ed
> ===
>
> It's then easier to google for when this happens to someone who runs
> non-patched kernels.
>
>> This fixes CVE-2020-14331.
>>
>> Reported-and-debugged-by: 张云海 <zhangyunhai@xxxxxxxxxxx>
>> Reported-and-debugged-by: Yang Yingliang <yangyingliang@xxxxxxxxxx>
>> Reported-by: Kyungtae Kim <kt0755@xxxxxxxxx>
>> Fixes: 15bdab959c9b ([PATCH] vgacon: Add support for soft scrollback)
>> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Greg KH <greg@xxxxxxxxx>
>> Cc: Solar Designer <solar@xxxxxxxxxxxx>
>> Cc: "Srivatsa S. Bhat" <srivatsa@xxxxxxxxxxxxx>
>> Cc: Anthony Liguori <aliguori@xxxxxxxxxx>
>> Cc: Yang Yingliang <yangyingliang@xxxxxxxxxx>
>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
>
> Oh, and we should:
> Cc: stable@xxxxxxxxxxxxxxx
>
>> Signed-off-by: Yunhai Zhang <zhangyunhai@xxxxxxxxxxx>
>> ---
>> drivers/video/console/vgacon.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
>> index 998b0de1812f..37b5711cd958 100644
>> --- a/drivers/video/console/vgacon.c
>> +++ b/drivers/video/console/vgacon.c
>> @@ -251,6 +251,10 @@ static void vgacon_scrollback_update(struct vc_data *c, int t, int count)
>> p = (void *) (c->vc_origin + t * c->vc_size_row);
>>
>> while (count--) {
>> + if ((vgacon_scrollback_cur->tail + c->vc_size_row) >
And git complains here:
.git/rebase-apply/patch:13: trailing whitespace.
if ((vgacon_scrollback_cur->tail + c->vc_size_row) >
warning: 1 line adds whitespace errors.
There is a space at the EOL.
>> + vgacon_scrollback_cur->size)
>> + vgacon_scrollback_cur->tail = 0;
>> +
>> scr_memcpyw(vgacon_scrollback_cur->data +
>> vgacon_scrollback_cur->tail,
>> p, c->vc_size_row);
>
> thanks,
>
--
js
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
