Received-SPF: pass (google.com: domain of linux-kernel+bounces-175016-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240506-b4-sio-scrollback-delta-v1-1-4164d162a2b8@google.com> <f7775510-09d8-41ef-97b2-0457e721a9ec@kernel.org>
In-Reply-To: <f7775510-09d8-41ef-97b2-0457e721a9ec@kernel.org>
From: Justin Stitt <justinstitt@google.com>
Date: Thu, 9 May 2024 15:01:14 -0700
Message-ID: <CAFhGd8qkYYQZi37Tsj-V5pN5S4xhcyUeAZj1of2kTXG+EtVMgg@mail.gmail.com>
Subject: Re: [PATCH] tty: vt: saturate scrollback_delta to avoid overflow
To: Jiri Slaby <jirislaby@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Nathan Chancellor <nathan@kernel.org>, 
	Bill Wendling <morbo@google.com>, linux-kernel@vger.kernel.org, 
	linux-serial@vger.kernel.org, llvm@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, May 8, 2024 at 11:31=E2=80=AFPM Jiri Slaby <jirislaby@kernel.org> w=
rote:
>
>
> And what is actually broken, given signed overflow is well defined under
> the -fwrapv wings?
>

well-defined code can still be broken.

We want to better safeguard against accidental overflow as this is the
root cause of many bugs/vulnerabilities. So, in this spirit, we
recently enhanced the signed-integer-overflow sanitizer and its
ability to function with -fwrapv enabled [1]. On the Kernel side of
things, Kees' tree [2] has done a lot of the groundwork to get this
sanitizer enabled and less noisy.

WIth all that being said, my solution to this problem in this
particular instance is not the most elegant, as you rightly pointed
out.


> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 9b5b98dfc8b4..b4768336868e 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -308,7 +308,14 @@ static inline void scrolldelta(int lines)
> >       /* FIXME */
> >       /* scrolldelta needs some kind of consistency lock, but the BKL w=
as
> >          and still is not protecting versus the scheduled back end */
> > -     scrollback_delta +=3D lines;
> > +
> > +     /* saturate scrollback_delta so that it never wraps around */
> > +     if (lines > 0 && unlikely(INT_MAX - lines < scrollback_delta))
> > +             scrollback_delta =3D INT_MAX;
> > +     else if (lines < 0 && unlikely(INT_MIN - lines > scrollback_delta=
))
> > +             scrollback_delta =3D INT_MIN;
> > +     else
> > +             scrollback_delta +=3D lines;
>
> NACK, this is horrid.

Agreed.

Does an implementation like this look any better?

static inline void scrolldelta(int lines)
{
        ...
        /* saturate scrollback_delta so that it never wraps around */
        if (lines > 0)
                scrollback_delta =3D min(scrollback_delta, INT_MAX -
lines) + lines;
        else
                scrollback_delta =3D max(scrollback_delta, INT_MIN -
lines) + lines;
        schedule_console_callback();
}

Accounting for the possibility of both underflow and overflow is what
is making this code a bit bloated but if this new approach looks good
enough I can send a v2.

>
> Introduce a helper for this in overflow.h if we have none yet.

We have these but for unsigned (size_t) types. I'm open to adding
signed_saturating_add(addend1, addend2, ceiling, floor) or something
similar.

>
> thanks,
> --
> js
> suse labs
>

[1]: https://github.com/llvm/llvm-project/pull/82432
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=
=3Ddev/v6.8-rc2/signed-overflow-sanitizer

Thanks
Justin