linux-kernel - Re: [PATCH v2] tty: Fix out-of-bound vmalloc access in imageblit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 15 Jun 2021 15:28:01 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Igor Matheus Andrade Torrente <igormtorrente@...il.com>
Cc:     jirislaby@...nel.org, linux-kernel@...r.kernel.org,
        syzbot+858dc7a2f7ef07c2c219@...kaller.appspotmail.com
Subject: Re: [PATCH v2] tty: Fix out-of-bound vmalloc access in imageblit

On Mon, Jun 14, 2021 at 10:18:59AM -0300, Igor Matheus Andrade Torrente wrote:
> This issue happens when a userspace program does an ioctl
> FBIOPUT_VSCREENINFO passing the fb_var_screeninfo struct
> containing only the fields xres, yres, and bits_per_pixel
> with values.
> 
> If this struct is the same as the previous ioctl, the
> vc_resize() detects it and doesn't call the resize_screen(),
> leaving the fb_var_screeninfo incomplete. And this leads to
> the updatescrollmode() calculates a wrong value to
> fbcon_display->vrows, which makes the real_y() return a
> wrong value of y, and that value, eventually, causes
> the imageblit to access an out-of-bound address value.
> 
> To solve this issue I made the resize_screen() be called
> even if the screen does not need any resizing, so it will
> "fix and fill" the fb_var_screeninfo independently.
> 
> Reported-and-tested-by: syzbot+858dc7a2f7ef07c2c219@...kaller.appspotmail.com
> Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@...il.com>
> ---
> v2: It Tries to avoid the problem found by Greg in the previous
>     patch.
> ---
>  drivers/tty/vt/vt.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index fa1548d4f94b..e522f9b249e5 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1219,8 +1219,10 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
>  	new_row_size = new_cols << 1;
>  	new_screen_size = new_row_size * new_rows;
>  
> -	if (new_cols == vc->vc_cols && new_rows == vc->vc_rows)
> -		return 0;
> +	if (new_cols == vc->vc_cols && new_rows == vc->vc_rows) {
> +		err = resize_screen(vc, new_cols, new_rows, user);
> +		return err;

Shouldn't this just be a single line change to:
		return resize_screen(vc, new_cols, new_rows, user);
?

But this also should get a big comment, as it looks odd that if the size
isn't changing, why this function should be called at all.  At first
glance at just the code, this looks wrong, so please document this
really well here.

thanks,

greg k-h