[PATCH] drivers/tty: Use get_user instead of dereferencing user pointer

[PATCH] drivers/tty: Use get_user instead of dereferencing user pointer

[Emil Goode]

We should use the get_user macro instead of dereferencing user
pointers directly.

This patch fixes the following sparse warning:
drivers/tty/n_tty.c:1648:51: warning:
	dereference of noderef expression

Signed-off-by: Emil Goode <emilgoode@gmail.com>
---
I'm a newbie so please review carefully.
Not sure if I should add error handling for the get_user call here.

 drivers/tty/n_tty.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 94b6eda..5ff63cc 100644
--- 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;
+	unsigned char ch;
 
 	retval = 0;
 	spin_lock_irqsave(&tty->read_lock, flags);
@@ -1645,7 +1646,8 @@ static int copy_from_read_buf(struct tty_struct *tty,
 		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))
+			get_user(ch, b[n-1]);
+			if (!tty->read_cnt && ch == EOF_CHAR(tty))
 				n--;
 		}
 		spin_unlock_irqrestore(&tty->read_lock, flags);
-- 
1.7.9.5

[PATCH] drivers/tty: Use get_user instead of dereferencing user pointer

[Emil Goode]

We should use the get_user macro instead of dereferencing user
pointers directly.

This patch fixes the following sparse warning:
drivers/tty/n_tty.c:1648:51: warning:
	dereference of noderef expression

Signed-off-by: Emil Goode <emilgoode@gmail.com>
---
I'm a newbie so please review carefully.
Not sure if I should add error handling for the get_user call here.

 drivers/tty/n_tty.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 94b6eda..5ff63cc 100644
--- 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;
+	unsigned char ch;
 
 	retval = 0;
 	spin_lock_irqsave(&tty->read_lock, flags);
@@ -1645,7 +1646,8 @@ static int copy_from_read_buf(struct tty_struct *tty,
 		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))
+			get_user(ch, b[n-1]);
+			if (!tty->read_cnt && ch = EOF_CHAR(tty))
 				n--;
 		}
 		spin_unlock_irqrestore(&tty->read_lock, flags);
-- 
1.7.9.5

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

[Alan Cox]

On Fri, 20 Apr 2012 17:52:34 +0200
Emil Goode <emilgoode@gmail.com> wrote:

> We should use the get_user macro instead of dereferencing user
> pointers directly.
> 
> This patch fixes the following sparse warning:
> drivers/tty/n_tty.c:1648:51: warning:
> 	dereference of noderef expression
> 
> Signed-off-by: Emil Goode <emilgoode@gmail.com>

So I gave it a five second glance and thought "must be a crazy newbie",
then I looked harder 8)

> I'm a newbie so please review carefully.
> Not sure if I should add error handling for the get_user call here.

You are correct, and it's been wrong for ages. You are also the first
person to my knowledge to actually dig into that sparse warning and fix
it!

>  		/* 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))
> +			get_user(ch, b[n-1]);
> +			if (!tty->read_cnt && ch = EOF_CHAR(tty))
>  				n--;
>  		}

The real question is what this code should be doing. The only case it can
be triggered as far as I can see is:

User provides buffer one byte shorter than the size passed to the syscall
Data is copied and copy_to_user returns 1 byte uncopied
n ends up as one

At this point we can take this path. What I don't understand at this
point is what this code path is actually *supposed* to do.

I think it should be testing against the value of n before the n -retval, and also checking the data it copied *from* in kernel space, not
the user size. The user data might be changed by another thread.

First problem therefore is to figure out what this code is supposed to be
doing. However you are right that the code is incorrect, and if it was a
simple bug your fix would also be correct.

It's not simple however, so can anyone work out or remember wtf the code
should be doing ???

Alan

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

[Alan Cox]

On Fri, 20 Apr 2012 17:52:34 +0200
Emil Goode <emilgoode@gmail.com> wrote:

> We should use the get_user macro instead of dereferencing user
> pointers directly.
> 
> This patch fixes the following sparse warning:
> drivers/tty/n_tty.c:1648:51: warning:
> 	dereference of noderef expression
> 
> Signed-off-by: Emil Goode <emilgoode@gmail.com>

So I gave it a five second glance and thought "must be a crazy newbie",
then I looked harder 8)

> I'm a newbie so please review carefully.
> Not sure if I should add error handling for the get_user call here.

You are correct, and it's been wrong for ages. You are also the first
person to my knowledge to actually dig into that sparse warning and fix
it!

>  		/* 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))
> +			get_user(ch, b[n-1]);
> +			if (!tty->read_cnt && ch == EOF_CHAR(tty))
>  				n--;
>  		}

The real question is what this code should be doing. The only case it can
be triggered as far as I can see is:

User provides buffer one byte shorter than the size passed to the syscall
Data is copied and copy_to_user returns 1 byte uncopied
n ends up as one

At this point we can take this path. What I don't understand at this
point is what this code path is actually *supposed* to do.

I think it should be testing against the value of n before the n -=
retval, and also checking the data it copied *from* in kernel space, not
the user size. The user data might be changed by another thread.

First problem therefore is to figure out what this code is supposed to be
doing. However you are right that the code is incorrect, and if it was a
simple bug your fix would also be correct.

It's not simple however, so can anyone work out or remember wtf the code
should be doing ???

Alan

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

[Jiri Slaby]

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@symas.com <hyc@symas.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

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

[Jiri Slaby]

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@symas.com <hyc@symas.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

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

[Howard Chu]

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@symas.com<hyc@symas.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/

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

[Howard Chu]

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@symas.com<hyc@symas.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/

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

[Howard Chu]

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@symas.com<hyc@symas.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/

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

[Howard Chu]

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@symas.com<hyc@symas.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/