Re: [PATCH -v2] memdup_user(): introduce

On Mon, 09 Mar 2009 11:30:18 +0800 Li Zefan <lizf@xxxxxxxxxxxxxx> wrote:

Andrew Morton wrote:
On Mon, 09 Mar 2009 10:22:08 +0800 Li Zefan <lizf@xxxxxxxxxxxxxx> wrote:

+EXPORT_SYMBOL(memdup_user);
Hi,

I like the general idea of this a lot; it will make things much less
error prone (and we can add some sanity checks on "len" to catch the
standard security holes around copy_from_user usage). I'd even also
want a memdup_array() like thing in the style of calloc().

However, I have two questions/suggestions for improvement:

I would like to question the use of the gfp argument here;
copy_from_user sleeps, so you can't use GFP_ATOMIC anyway.
You can't use GFP_NOFS etc, because the pagefault path will happily do
things that are equivalent, if not identical, to GFP_KERNEL.

So the only value you can pass in correctly, as far as I can see, is
GFP_KERNEL. Am I wrong?

Right! I just dug and found a few kmalloc(GFP_ATOMIC/GFP_NOFS)+copy_from_user(),
so we have one more reason to use this memdup_user().

gack, those callsites are probably buggy. Where are they?


Yes, either buggy or should use GFP_KERNEL.

All are in -mm only, except the first one:

drivers/isdn/i4l/isdn_common.c:
struct sk_buff *skb = alloc_skb(hl + len, GFP_ATOMIC);
...
if (copy_from_user(skb_put(skb, len), buf, len)) {

Bug. Should be GFP_KERNEL, or copy_from_user() is incorrect in this
context.


net/irda/af_irda.c:
ias_opt = kmalloc(sizeof(struct irda_ias_set), GFP_ATOMIC);
...
if (copy_from_user(ias_opt, optval, optlen)) {

Bug. Should be GFP_KERNEL, or copy_from_user() is incorrect in this
context.


fs/btrfs/ioctl.c:
vol_args = kmalloc(sizeof(*vol_args), GFP_NOFS);
...
if (copy_from_user(vol_args, arg, sizeof(*vol_args))) {

Bug. Should be GFP_KERNEL, or copy_from_user() is incorrect in this
context.

fs/ocfs2/dlm/dlmfs.c:
lvb_buf = kmalloc(writelen, GFP_NOFS);
...
bytes_left = copy_from_user(lvb_buf, buf, writelen);

Bug. Should be GFP_KERNEL, or copy_from_user() is incorrect in this
context.


net/sunrpc/auth_gss/auth_gss.c:
buf = kmalloc(mlen, GFP_NOFS);
...
if (copy_from_user(buf, src, mlen))

Bug. Should be GFP_KERNEL, or copy_from_user() is incorrect in this
context.

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

Relevant Pages

  • Re: Id like to know some meaning of shell scripts?
    ... The $1 here too seems like typo and, or bug to me; ... Why do you think $1 is incorrect? ... yep, is valid as per syntax, but incorrect as per context and, or the ... requite logic and that's what is known as a bug. ...
    (comp.os.linux.misc)
  • Re: Id like to know some meaning of shell scripts?
    ... The $1 here too seems like typo and, or bug to me; ... Why do you think $1 is incorrect? ... yep, is valid as per syntax, but incorrect as per context and, or the ... requite logic and that's what is known as a bug. ...
    (comp.os.linux.misc)
  • Re: "Friendly Premises"
    ... >> That statement is incorrect. ... one statement, are *real world.* (Excluding textbook examples, ... he excerpted "true in that set of axioms" out of context, ... Now getting back to Torkel's assertion at the top, ...
    (sci.logic)
  • Re: Blockbuster picks Blu-Ray - questions
    ... However, that's the *only* context ... To deviate from those methods is to write a sentence that is incorrect. ... I never labeled a *writer* as wrong, just the writing. ... Blu-ray Software Sales Surpass HD-DVD ...
    (rec.arts.movies.current-films)
  • Re: Blockbuster picks Blu-Ray - questions
    ... trotsky's use of the word "software" included digital downloads. ... However, that context doesn't include ... context that trotsky's usage lacked. ... deliberately correct and incorrect sentences, ...
    (rec.arts.movies.current-films)