Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 6 Mar 2023 08:32:11 +0100 | From | Jiri Slaby <> | Subject | Re: [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
| ![\](/images/icornerr.gif) |