linux-kernel - Re: [PATCH] fbmem: prevent potential use-after-free issues with console

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2711de96-fcbe-5611-657a-ab29becd2ff6@gmx.de>
Date:   Mon, 2 Jan 2023 16:28:57 +0100
From:   Helge Deller <deller@....de>
To:     Hang Zhang <zh.nvgt@...il.com>
Cc:     Daniel Vetter <daniel@...ll.ch>,
        Javier Martinez Canillas <javierm@...hat.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Sam Ravnborg <sam@...nborg.org>,
        Alex Deucher <alexander.deucher@....com>,
        linux-fbdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fbmem: prevent potential use-after-free issues with
 console_lock()

On 12/30/22 07:35, Hang Zhang wrote:
> In do_fb_ioctl(), user specified "fb_info" can be freed in the callee
> fbcon_get_con2fb_map_ioctl() -> set_con2fb_map() ->
> con2fb_release_oldinfo(), this free operation is protected by
> console_lock() in fbcon_set_con2fb_map_ioctl(), it also results in
> the change of certain states such as "minfo->dead" in matroxfb_remove(),
> so that it can be checked to avoid use-after-free before the use sites
> (e.g., the check at the beginning of matroxfb_ioctl()). However,
> the problem is that the use site is not protected by the same locks
> as for the free operation, e.g., "default" case in do_fb_ioctl()
> can lead to "matroxfb_ioctl()" but it's not protected by console_lock(),
> which can invalidate the aforementioned state set and check in a
> concurrent setting.
>
> Prevent the potential use-after-free issues by protecting the "default"
> case in do_fb_ioctl() with console_lock(), similarly as for many other
> cases like "case FBIOBLANK" and "case FBIOPAN_DISPLAY".
>
> Signed-off-by: Hang Zhang <zh.nvgt@...il.com>

applied to fbdev git tree.

Thanks,
Helge

> ---
>   drivers/video/fbdev/core/fbmem.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 1e70d8c67653..8b1a1527d18a 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1182,6 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>   		console_unlock();
>   		break;
>   	default:
> +		console_lock();
>   		lock_fb_info(info);
>   		fb = info->fbops;
>   		if (fb->fb_ioctl)
> @@ -1189,6 +1190,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>   		else
>   			ret = -ENOTTY;
>   		unlock_fb_info(info);
> +		console_unlock();
>   	}
>   	return ret;
>   }