Re: [PATCH] tty: vt: Fix slab-out-of-bounds write in do_con_write

[Haocheng Yu]

I only ran my fuzzer on v6.18, so I'm unsure what commit caused the issue.

I tried analyzing the cause, but the crash report provided limited information.
I tried directly analyzing the code, but a possible race condition
with vc_do_resize()
is unlikely due to the console_lock, and the state.x out-of-bounds
issue is invalidated
by the restrictions in gotoxy(). Furthermore, no reproducer were
provided, making it
difficult to verify some of my hypotheses.

My initial thought was that although the specific cause was uncertain,
since scr_writew()
causes slab-out-of-bounds writes, adding a check before scr_writew()
would solve the
problem. After reading your explanation, I realize my thinking was
quite naive. Therefore,
I think I may not be able to provide a better patch at the moment.
Please ignore this patch.

BTW, I ran `./scripts/checkpatch.pl --strict` indeed, but it didn't
warn me about the variable
created. I'll pay attention next time.

Anyway, thanks for reviewing my patch.

Best regards,
Haocheng

> What commit caused this problem to show up?
>
> And without more information, or a reproducer, I'm a bit loath to take
> this change.
>
> > ---
> >  drivers/tty/vt/vt.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 6e0089b85c27..95d860f09837 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -3138,6 +3138,13 @@ static int vc_con_write_normal(struct vc_data *vc, int tc, int c,
> >                             (tc &  0xff);
> >               tc |= (vc_attr << 8) & ~himask;
> >
> > +             unsigned long end = vc->vc_origin + vc->vc_screenbuf_size;
>
> Ideally do not create new variables in the middle of a function,
> checkpatch should have warned about this.
>
> > +
> > +             if (WARN_ON_ONCE(vc->vc_screenbuf_size < 2 ||
> > +                              end < vc->vc_origin ||
> > +                              vc->vc_pos < vc->vc_origin ||
> > +                              vc->vc_pos > end - 2))
>
> That's not good, if panic-on-warn is enabled, as it is in a few billion
> Linux systems, you just rebooted the machine, turning a simple overwrite
> into a denial-of-service, not fixing anything at all, but making it
> worse :(
>
> > +                     return -1;
>
> Do not make up error numbers :(
>
> thanks,
>
> greg k-h