linux-kernel - Re: [regression] [bisected] commit 6bb6fa6908ebd3cb4e14cd4f0ce272ec885d2eb0 corrupts data sent via pseudoterminal device

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] [day] [month] [year] [list]
Message-ID: <563463792.1252370.1715694700555@mail.yahoo.com>
Date: Tue, 14 May 2024 13:51:40 +0000 (UTC)
From: Vadym Krevs <vkrevs@...oo.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Andy Shevchenko <andy.shevchenko@...il.com>, 
	Bagas Sanjaya <bagasdotme@...il.com>, 
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, 
	Linux Regressions <regressions@...ts.linux.dev>, 
	Linux Serial <linux-serial@...r.kernel.org>, 
	Gilles Buloz <gilles.buloz@...tron.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Jiri Slaby <jirislaby@...nel.org>
Subject: Re: [regression] [bisected] commit
 6bb6fa6908ebd3cb4e14cd4f0ce272ec885d2eb0 corrupts data sent via
 pseudoterminal device

On Tuesday, 14 May 2024 at 14:30:54 BST, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com> wrote:
 
> On Tue, 14 May 2024, Vadym Krevs wrote:
> 
> > On Tuesday, 14 May 2024 at 12:03:25 BST, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com> wrote:
> >
> > > On Tue, 14 May 2024, Andy Shevchenko wrote:
> > >
> > > > On Tue, May 14, 2024 at 12:28 PM Vadym Krevs <vkrevs@...oo.com> wrote:
> > > > >
> > > > > It's a standard setup for an out-of-the box default install of openSUSE 15.5 with KDE. All tests done in Konsole with bash as shell.
> > > > >
> > > > > stty -a -F /dev/pts/1
> > > > > speed 38400 baud; rows 57; columns 217; line = 0;
> > > > > intr = ^C; quit = ^; erase = ^?; kill = ^U; eof = ^D; eol = <undef>; eol2 = <undef>; swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R; werase = ^W; lnext = ^V; discard = ^O; min = 1; time = 0;
> > > > > -parenb -parodd -cmspar cs8 -hupcl -cstopb cread -clocal -crtscts
> > > > > -ignbrk -brkint -ignpar -parmrk -inpck -istrip -inlcr -igncr icrnl ixon ixoff -iuclc -ixany -imaxbel iutf8
> > > > > opost -olcuc -ocrnl onlcr -onocr -onlret -ofill -ofdel nl0 cr0 tab0 bs0 vt0 ff0
> > > > > isig icanon iexten echo echoe echok -echonl -noflsh -xcase -tostop -echoprt echoctl echoke -flusho -extproc
> > > >
> > > > Thank you!
> > > >
> > > > Yeah. SW flow control is enabled, but I don't see which character is
> > > > being used for that. Anyway, let's give Ilpo a chance to look into
> > > > this.
> > >
> > > Thanks a lot for pinpointing the commit with bisect. It turns out this
> > > is a quite bad corruption bug and I'm quite surprised I didn't see (or
> > > notice) it while testing the patch.
> > >
> > > Could you please test and confirm the patch below fixes the issue?
> > > --
> > > [PATCH] tty: n_tty: Fix buffer offsets when looked ahead is used
> > >
> > > When lookahead has "consumed" some characters (la_count > 0),
> > > n_tty_receive_buf_standard() and n_tty_receive_buf_closing() for
> > > characters beyond the la_count are given wrong cp/fp offsets which
> > > leads to duplicating and losing some characters.
> > >
> > > If la_count > 0, correct buffer pointers and make count consistency too
> > > (the latter is not strictly necessary to fix the issue but seems more
> > > logical to adjust all variables immediately to keep state consistent).
> > >
> > > Reported-by: Vadym Krevs <vkrevs@...oo.com>
> > > Fixes: 6bb6fa6908eb ("tty: Implement lookahead to process XON/XOFF timely")
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218834
> > > Cc: stable@...r.kernel.org
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > > ---
> > > drivers/tty/n_tty.c | 22 ++++++++++++++++------
> > > 1 file changed, 16 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> > > index f252d0b5a434..5e9ca4376d68 100644
> > > --- a/drivers/tty/n_tty.c
> > > +++ b/drivers/tty/n_tty.c
> > > @@ -1619,15 +1619,25 @@ static void __receive_buf(struct tty_struct *tty, const u8 *cp, const u8 *fp,
> > > else if (ldata->raw || (L_EXTPROC(tty) && !preops))
> > > n_tty_receive_buf_raw(tty, cp, fp, count);
> > > else if (tty->closing && !L_EXTPROC(tty)) {
> > > -        if (la_count > 0)
> > > +        if (la_count > 0) {
> > > n_tty_receive_buf_closing(tty, cp, fp, la_count, true);
> > > -        if (count > la_count)
> > > -            n_tty_receive_buf_closing(tty, cp, fp, count - la_count, false);
> > > +            cp += la_count;
> > > +            if (fp)
> > > +                fp += la_count;
> > > +            count -= la_count;
> > > +        }
> > > +        if (count > 0)
> > > +            n_tty_receive_buf_closing(tty, cp, fp, count, false);
> > > } else {
> > > -        if (la_count > 0)
> > > +        if (la_count > 0) {
> > > n_tty_receive_buf_standard(tty, cp, fp, la_count, true);
> > > -        if (count > la_count)
> > > -            n_tty_receive_buf_standard(tty, cp, fp, count - la_count, false);
> > > +            cp += la_count;
> > > +            if (fp)
> > > +                fp += la_count;
> > > +            count -= la_count;
> > > +        }
> > > +        if (count > 0)
> > > +            n_tty_receive_buf_standard(tty, cp, fp, count, false);
> > >
> > > flush_echoes(tty);
> > > if (tty->ops->flush_chars)
> > > --
> > > 2.39.2
> >
> > Yes, I've tested the patch against the 6.9.0-rc7-local-00012-gdccb07f2914c kernel (last commit 45db3ab70092637967967bfd8e6144017638563c from May 8th) and it works just fine.
> >
> > Thank you very much for fixing the problem so quicky.
> >
> > Kind regards,
> > Vadym
> >
> > P.S.: Hopefully, Yahoo mail has actually sent this reply as plain text.
> 
> Thanks for testing.
> 
> Can I put your Tested-by tag into the fix?
> 
> 
> --
> i.

Yes, of course. 

Tested-by: Vadym Krevs <vkrevs@...oo.com>