Message-ID: <008a3a1d-1908-6aea-0fae-e15b4eddff02@kernel.org>
Date:   Thu, 30 Jul 2020 09:38:24 +0200
From:   Jiri Slaby <jirislaby@...nel.org>
To:     张云海 <zhangyunhai@...ocus.com>,
        Solar Designer <solar@...nwall.com>
Cc:     b.zolnierkie@...sung.com,
        Yang Yingliang <yangyingliang@...wei.com>,
        Kyungtae Kim <kt0755@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg KH <greg@...ah.com>,
        "Srivatsa S. Bhat" <srivatsa@...il.mit.edu>,
        Anthony Liguori <aliguori@...zon.com>,
        xiao.zhang@...driver.com,
        DRI devel <dri-devel@...ts.freedesktop.org>,
        Linux Fbdev development list <linux-fbdev@...r.kernel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] vgacon: fix out of bounds write to the scrollback buffer

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@...ocus.com>
>> 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@...ocus.com>
>> Reported-and-debugged-by: Yang Yingliang <yangyingliang@...wei.com>
>> Reported-by: Kyungtae Kim <kt0755@...il.com>
>> Fixes: 15bdab959c9b ([PATCH] vgacon: Add support for soft scrollback)
>> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
>> Cc: Greg KH <greg@...ah.com>
>> Cc: Solar Designer <solar@...nwall.com>
>> Cc: "Srivatsa S. Bhat" <srivatsa@...il.mit.edu>
>> Cc: Anthony Liguori <aliguori@...zon.com>
>> Cc: Yang Yingliang <yangyingliang@...wei.com>
>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
> 
> Oh, and we should:
> Cc: stable@...r.kernel.org
> 
>> Signed-off-by: Yunhai Zhang <zhangyunhai@...ocus.com>
>> ---
>>  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

Powered by blists - more mailing lists