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: <4F92BE52.4020908@symas.com>
Date:	Sat, 21 Apr 2012 07:04:02 -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

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.

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