[PATCH] vc_screen: reload vc pointer before if (ret) in vcs_write() to avoid UAF

[PATCH] vc_screen: reload vc pointer before if (ret) in vcs_write() to avoid UAF

[Yi Yang]

The reload of 'vc' added by commit 8fb9ea65c9d1 ("vc_screen: reload load
of struct vc_data pointer in vcs_write() to avoid UAF") sits after the
'if (ret)' block, so the copy-failure break path exits the loop without
reloading vc. If the vc was kfree()'d via vc_port_destruct during the
unlocked copy_from_user() window, the post-loop
'if (written && vc) vcs_scr_updated(vc)' is reached with a stale
non-NULL vc. The '&& vc' guard from commit a287620312dc ("vc_screen:
fix null-ptr-deref in vcs_notifier() during concurrent vcs_write") only
handles the NULL case, not this stale-non-NULL case; vcs_notifier() then
reads param->vc->vc_num from freed memory:

  BUG: KASAN: slab-use-after-free in vcs_notifier+0x7c/0xd0
  Read of size 2 at addr ffff888007149190
  Call Trace:
   vcs_notifier+0x7c/0xd0
   atomic_notifier_call_chain+0x70/0xa0
   vcs_scr_updated+0x77/0xa0
   vcs_write+0x71b/0x7e0
  Allocated by task: vc_allocate -> con_install -> tty_open
  Freed by task: kfree <- vt_ioctl (VT_DISALLOCATE -> vc_port_destruct)

Move the reload to immediately after console_lock(), before 'if (ret)',
so every break path below passes a fresh vc to the post-loop
vcs_scr_updated().

Fixes: a287620312dc ("vc_screen: fix null-ptr-deref in vcs_notifier() during concurrent vcs_write")
Cc: stable@vger.kernel.org
Signed-off-by: Yi Yang <yiyang13@huawei.com>
---
 drivers/tty/vt/vc_screen.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index bf1502fd5bd4..79453abcf4d9 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -631,6 +631,18 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 		ret = copy_from_user(con_buf, buf, this_round);
 		console_lock();
 
+		/* The vc might have been freed or vcs_size might have changed
+		 * while we slept to grab the user buffer; recheck here, before
+		 * if (ret), so every break path below passes a fresh vc to the
+		 * post-loop vcs_scr_updated(). Return data written so far.
+		 */
+		vc = vcs_vc(inode, &viewed);
+		if (!vc) {
+			if (written)
+				break;
+			return -ENXIO;
+		}
+
 		if (ret) {
 			this_round -= ret;
 			if (!this_round) {
@@ -642,17 +654,6 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 				return -EFAULT;
 			}
 		}
-
-		/* The vc might have been freed or vcs_size might have changed
-		 * while we slept to grab the user buffer, so recheck.
-		 * Return data written up to now on failure.
-		 */
-		vc = vcs_vc(inode, &viewed);
-		if (!vc) {
-			if (written)
-				break;
-			return -ENXIO;
-		}
 		size = vcs_size(vc, attr, false);
 		if (size < 0) {
 			if (written)
-- 
2.25.1

Re: [PATCH] vc_screen: reload vc pointer before if (ret) in vcs_write() to avoid UAF

[sashiko-bot]

> The reload of 'vc' added by commit 8fb9ea65c9d1 ("vc_screen: reload load
> of struct vc_data pointer in vcs_write() to avoid UAF") sits after the
> 'if (ret)' block, so the copy-failure break path exits the loop without
> reloading vc. If the vc was kfree()'d via vc_port_destruct during the
> unlocked copy_from_user() window, the post-loop
> 'if (written && vc) vcs_scr_updated(vc)' is reached with a stale
> non-NULL vc. The '&& vc' guard from commit a287620312dc ("vc_screen:
> fix null-ptr-deref in vcs_notifier() during concurrent vcs_write") only
> handles the NULL case, not this stale-non-NULL case; vcs_notifier() then
> reads param->vc->vc_num from freed memory:
> 
>   BUG: KASAN: slab-use-after-free in vcs_notifier+0x7c/0xd0
>   Read of size 2 at addr ffff888007149190
>   Call Trace:
>    vcs_notifier+0x7c/0xd0
>    atomic_notifier_call_chain+0x70/0xa0
>    vcs_scr_updated+0x77/0xa0
>    vcs_write+0x71b/0x7e0
>   Allocated by task: vc_allocate -> con_install -> tty_open
>   Freed by task: kfree <- vt_ioctl (VT_DISALLOCATE -> vc_port_destruct)
> 
> Move the reload to immediately after console_lock(), before 'if (ret)',
> so every break path below passes a fresh vc to the post-loop
> vcs_scr_updated().
> 
> Fixes: a287620312dc ("vc_screen: fix null-ptr-deref in vcs_notifier() during concurrent vcs_write")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yi Yang <yiyang13@huawei.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901113131.2760010-1-yiyang13@huawei.com?part=1