lkml.org 
[lkml]   [2023]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [syzbot] [hardening?] [mm?] BUG: bad usercopy in con_font_op
On 05. 03. 23, 18:54, Samuel Thibault wrote:
> 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?

It's as simple as adding:
Reported-by: syzbot+3af17071816b61e807ed@syzkaller.appspotmail.com
to the tags.

> 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")

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

> Signed-off-by: Samuel Thibault <samuel.thibault@ens-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)

--
js

\
 
 \ /
  Last update: 2023-03-27 00:44    [W:0.044 / U:0.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site