linux-hardening - Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in con_font

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: <20230305175457.kp6b5lmwwdxw4ii6@begin>
Date:   Sun, 5 Mar 2023 18:54:57 +0100
From:   Samuel Thibault <samuel.thibault@...-lyon.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     syzbot <syzbot+3af17071816b61e807ed@...kaller.appspotmail.com>,
        akpm@...ux-foundation.org, linux-hardening@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        syzkaller-bugs@...glegroups.com, Jiri Slaby <jirislaby@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in con_font_op

Kees Cook, le ven. 03 mars 2023 14:07:04 -0800, a ecrit:
> #define max_font_width  64
> #define max_font_height 128
> #define max_font_glyphs 512
> #define max_font_size   (max_font_glyphs*max_font_width*max_font_height)
> 	...
>         font.data = kvmalloc(max_font_size, GFP_KERNEL);
> 	...
>         if (op->data && copy_to_user(op->data, font.data, c))
>                 rc = -EFAULT;
> 
> it is correctly seeing "c" (4194560 in the report) as larger than
> "max_font_size" (4194304, seen reported by "folio_size(folio)"). The
> "c" calculation comes from:
> 
>         unsigned int vpitch = op->op == KD_FONT_OP_GET_TALL ? op->height : 32;
> 	...
>                 rc = vc->vc_sw->con_font_get(vc, &font, vpitch);
> 	...
>         c = (font.width+7)/8 * vpitch * font.charcount;
> 
> So yes, 4194560 is larger than 4194304, and a memory exposure was,
> in fact, blocked here.
> 
> Given the recent work in this area, I'm not sure which calculation is
> wrong, max_font_size or c. Samuel?

They are not wrong. It's the vpitch value (coming from userland's
op.height) which is out of bound and missing a check.

The patch below should be fixing it, could you check?

I don't know how I am supposed to properly reference the syzbot report
etc., could somebody used to the process handle submitting the fix?

Samuel

VT: Protect KD_FONT_OP_GET_TALL from unbound access

In ioctl(KD_FONT_OP_GET_TALL), userland tells through op->height which
vpitch should be used to copy over the font. In con_font_get, we were
not checking that it is within the maximum height value, and thus
userland could make the vc->vc_sw->con_font_get(vc, &font, vpitch);
call possibly overflow the allocated max_font_size bytes, and the
copy_to_user(op->data, font.data, c) call possibly read out of that
allocated buffer.

By checking vpitch against max_font_height, the max_font_size buffer
will always be large enough for the vc->vc_sw->con_font_get(vc, &font,
vpitch) call (since we already prevent loading a font larger than that),
and c = (font.width+7)/8 * vpitch * font.charcount will always remain
below max_font_size.

Fixes: 24d69384bcd3 ("VT: Add KD_FONT_OP_SET/GET_TALL operations")
Signed-off-by: Samuel Thibault <samuel.thibault@...-lyon.org>

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 57a5c23b51d4..3c2ea9c098f7 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4545,6 +4545,9 @@ static int con_font_get(struct vc_data *vc, struct console_font_op *op)
 	int c;
 	unsigned int vpitch = op->op == KD_FONT_OP_GET_TALL ? op->height : 32;

+	if (vpitch > max_font_height)
+		return -EINVAL;
+
 	if (op->data) {
 		font.data = kvmalloc(max_font_size, GFP_KERNEL);
 		if (!font.data)