[PATCH] vc_screen: fix null-ptr-deref in vcs_notifier() during concurrent vcs_write

[PATCH] vc_screen: fix null-ptr-deref in vcs_notifier() during concurrent vcs_write

[Yi Yang]

A KASAN null-ptr-deref was observed in vcs_notifier():

BUG: KASAN: null-ptr-deref in vcs_notifier+0x98/0x130
Read of size 2 at addr qmp_cmd_name: qmp_capabilities, arguments: {}

The issue is a race condition in vcs_write(). When the console_lock is
temporarily dropped (to copy data from userspace), the vc_data pointer
obtained from vcs_vc() may become stale. After re-acquiring the lock,
vcs_vc() is called again to re-validate the pointer. If the vc has been
deallocated in the meantime, vcs_vc() returns NULL, and the while loop
breaks (with written > 0). However, after the loop, vcs_scr_updated(vc)
is still called with the now-NULL vc pointer, leading to a null pointer
dereference in the notifier chain (vcs_notifier dereferences param->vc).

Fix this by adding a NULL check for vc before calling vcs_scr_updated().

Fixes: 8fb9ea65c9d1 ("vc_screen: reload load of struct vc_data pointer in vcs_write() to avoid UAF")
Cc: stable@vger.kernel.org
Signed-off-by: Yi Yang <yiyang13@huawei.com>
---
 drivers/tty/vt/vc_screen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index 4d2d46c95fef..7d40eacc21b3 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -686,7 +686,7 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 	}
 	*ppos += written;
 	ret = written;
-	if (written)
+	if (written && vc)
 		vcs_scr_updated(vc);
 
 	return ret;
-- 
2.25.1

Re: [PATCH] vc_screen: fix null-ptr-deref in vcs_notifier() during concurrent vcs_write

[Jiri Slaby]

On 04. 06. 26, 8:07, Yi Yang wrote:
> A KASAN null-ptr-deref was observed in vcs_notifier():
> 
> BUG: KASAN: null-ptr-deref in vcs_notifier+0x98/0x130
> Read of size 2 at addr qmp_cmd_name: qmp_capabilities, arguments: {}
> 
> The issue is a race condition in vcs_write(). When the console_lock is
> temporarily dropped (to copy data from userspace), the vc_data pointer
> obtained from vcs_vc() may become stale. After re-acquiring the lock,
> vcs_vc() is called again to re-validate the pointer. If the vc has been
> deallocated in the meantime, vcs_vc() returns NULL, and the while loop
> breaks (with written > 0). However, after the loop, vcs_scr_updated(vc)
> is still called with the now-NULL vc pointer, leading to a null pointer
> dereference in the notifier chain (vcs_notifier dereferences param->vc).
> 
> Fix this by adding a NULL check for vc before calling vcs_scr_updated().

And vc is still protected by console_lock at that point.

> Fixes: 8fb9ea65c9d1 ("vc_screen: reload load of struct vc_data pointer in vcs_write() to avoid UAF")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yi Yang <yiyang13@huawei.com>

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

> ---
>   drivers/tty/vt/vc_screen.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> index 4d2d46c95fef..7d40eacc21b3 100644
> --- a/drivers/tty/vt/vc_screen.c
> +++ b/drivers/tty/vt/vc_screen.c
> @@ -686,7 +686,7 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>   	}
>   	*ppos += written;
>   	ret = written;
> -	if (written)
> +	if (written && vc)
>   		vcs_scr_updated(vc);
>   
>   	return ret;


-- 
js
suse labs