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

Message-ID: <4F92C345.5060604@symas.com>
Date:	Sat, 21 Apr 2012 07:25:09 -0700
From:	Howard Chu <hyc@...as.com>
To:	Jiri Slaby <jslaby@...e.cz>
CC:	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Emil Goode <emilgoode@...il.com>, gregkh@...uxfoundation.org,
	kernel-janitors@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jiri Slaby <jirislaby@...il.com>
Subject: Re: [PATCH] drivers/tty: Use get_user instead of dereferencing user
 pointer

Howard Chu wrote:
> Jiri Slaby wrote:
>> 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.
>
> Correct.
>
>> 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?
>
> Hmmm, probably not expected, no. The intent was to pass the EOF character
> through if it's part of a non-empty input line. But if the EOF is the first
> character on an input line, it should be treated as an EOF and no data
> returned from the read.

Been a while since I've thought about this. Perhaps it should simply have 
checked if (tty->read_cnt == 1).

>> 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,
>
>


-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/
--
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/