linux-kernel - Re: [PATCH] drivers/tty: Use get_user instead of dereferencing user pointer

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4F927804.4090208@suse.cz>
Date:	Sat, 21 Apr 2012 11:04:04 +0200
From:	Jiri Slaby <jslaby@...e.cz>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
CC:	Emil Goode <emilgoode@...il.com>, gregkh@...uxfoundation.org,
	kernel-janitors@...r.kernel.org, linux-kernel@...r.kernel.org,
	hyc@...as.com, Jiri Slaby <jirislaby@...il.com>
Subject: Re: [PATCH] drivers/tty: Use get_user instead of dereferencing user
 pointer

On 04/20/2012 11:00 PM, Alan Cox wrote:
> It's not simple however, so can anyone work out or remember wtf the code
> should be doing ???

Huh.

The code was added by:
commit 26df6d13406d1a53b0bda08bd712f1924affd7cd
Author: hyc@...as.com <hyc@...as.com>
Date:   Tue Jun 22 10:14:49 2010 -0700

    tty: Add EXTPROC support for LINEMODE

====

The code is now:

retval = copy_to_user(*b, &tty->read_buf[tty->read_tail], n);
n -= retval;
tty_audit_add_data(tty, &tty->read_buf[tty->read_tail], n);
spin_lock_irqsave(&tty->read_lock, flags);
tty->read_tail = (tty->read_tail + n) & (N_TTY_BUF_SIZE-1);
tty->read_cnt -= n;
if (L_EXTPROC(tty) && tty->icanon && n == 1) {
        if (!tty->read_cnt && (*b)[n-1] == EOF_CHAR(tty))
                n--;
}

====

n after "n -= retval" means number of successfully copied chars. So the
test "n == 1" along with "!tty->read_cnt" actually should ensure we
copied everything and that is exactly one char. Further we test if that
one is EOF. If so, ignore that char by pretending we copied nothing.

However the implementation does not count with buffer wrapped like:
EOF..........................something
                             ^----- tail

Here, the first call to copy_from_read_buf copies "something" and the
second one is to copy single EOF. But that would be ignored! Is this
expected?


So to fix the user buffer dereference, the following diff should help.
In any case the wrapped buffer is still to be fixed... (Or ignored.)
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1630,6 +1630,7 @@ static int copy_from_read_buf(struct tty_struct *tty,
        int retval;
        size_t n;
        unsigned long flags;
+       bool is_eof;

        retval = 0;
        spin_lock_irqsave(&tty->read_lock, flags);
@@ -1639,15 +1640,15 @@ static int copy_from_read_buf(struct tty_struct
*tty,
        if (n) {
                retval = copy_to_user(*b,
&tty->read_buf[tty->read_tail], n);
                n -= retval;
+               is_eof = n == 1 &&
+                       tty->read_buf[tty->read_tail] == EOF_CHAR(tty);
                tty_audit_add_data(tty, &tty->read_buf[tty->read_tail], n);
                spin_lock_irqsave(&tty->read_lock, flags);
                tty->read_tail = (tty->read_tail + n) & (N_TTY_BUF_SIZE-1);
                tty->read_cnt -= n;
                /* Turn single EOF into zero-length read */
-               if (L_EXTPROC(tty) && tty->icanon && n == 1) {
-                       if (!tty->read_cnt && (*b)[n-1] == EOF_CHAR(tty))
-                               n--;
-               }
+               if (L_EXTPROC(tty) && tty->icanon && is_eof &&
!tty->read_cnt)
+                       n = 0;
                spin_unlock_irqrestore(&tty->read_lock, flags);
                *b += n;
                *nr -= n;

thanks,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/