From: Alan Cox <alan@lxorguk.ukuu.org.uk>
To: Emil Goode <emilgoode@gmail.com>
Cc: gregkh@linuxfoundation.org, kernel-janitors@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drivers/tty: Use get_user instead of dereferencing user pointer
Date: Fri, 20 Apr 2012 22:00:10 +0100 [thread overview]
Message-ID: <20120420220010.0fa29bee@pyramind.ukuu.org.uk> (raw)
In-Reply-To: <1334937154-23037-1-git-send-email-emilgoode@gmail.com>
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
next prev parent reply other threads:[~2012-04-20 20:57 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-20 15:52 [PATCH] drivers/tty: Use get_user instead of dereferencing user pointer Emil Goode
2012-04-20 21:00 ` Alan Cox [this message]
2012-04-21 9:04 ` Jiri Slaby
2012-04-21 14:04 ` Howard Chu
2012-04-21 14:25 ` Howard Chu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120420220010.0fa29bee@pyramind.ukuu.org.uk \
--to=alan@lxorguk.ukuu.org.uk \
--cc=emilgoode@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox