lkml.org 
[lkml]   [2008]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

Zev Weiss wrote:
> Andrew Morton wrote:
> > On Sat, 23 Aug 2008 01:10:21 -0700 Zev Weiss <zevweiss@gmail.com> wrote:
> >
> >> Hmm. Well, I may be misunderstanding what you're saying (again, I'm
> very much
> >> a newbie to kernelspace), but I *think* the "copying four u32's out to
> >> userspace" thing isn't really a problem with my patch. It does
> certainly copy
> >> those four u32's, but given that `ur' (struct mtd_region_info_user) is
> >> initialized by copying from userspace, its fourth u32 (the
> `regionindex'
> >> member) should be identical when copied back out to userspace, given
> that it's
> >> not touched in the memberwise modification of the struct.
> >
> > OK, that's fortuitously bug-free in single-threaded userspace but
> > fantastically-improbably-buggy if userspace is threaded.
> >
> > But it's something the kernel shouldn't be doing.
> >
>
> Ah, good point -- that hadn't occurred to me at all. Though it looks
> pretty
> clumsy/simpleminded to me, I guess something like this would avoid
> copying and rewriting the fourth u32 ("Well, duh" may be the appropriate
> response here):
>
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 13cc67a..424f318 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -410,16 +410,20 @@ static int mtd_ioctl(struct inode *inode, struct
> file *file,
>
> case MEMGETREGIONINFO:
> {
> - struct region_info_user ur;
> + u32 ur_idx;
> + struct mtd_erase_region_info *kr;
> + struct region_info_user *ur = (struct region_info_user
> *) argp;
>
> - if (copy_from_user(&ur, argp, sizeof(struct
> region_info_user)))
> + if (get_user(ur_idx,&(ur->regionindex)))
> return -EFAULT;
>
> - if (ur.regionindex >= mtd->numeraseregions)
> - return -EINVAL;
> - if (copy_to_user(argp,
> &(mtd->eraseregions[ur.regionindex]),
> - sizeof(struct mtd_erase_region_info)))
> + kr = &(mtd->eraseregions[ur_idx]);
> +
> + if (put_user(kr->offset, &(ur->offset))
> + || put_user(kr->erasesize, &(ur->erasesize))
> + || put_user(kr->numblocks, &(ur->numblocks)))
> return -EFAULT;
> +
> break;
> }
>
>
> [Note: this is not even so much as compile-tested, and will remain that way
> until Monday, but I think you get the picture.]
>

Well, for what little it's probably worth, I've now tested this patch, with no
resulting surprises (seems to work fine for me). Though I see now that, much to
my chagrin, something in my email chain seems to have spacified my code. If
there's any need I can resend appropriately.

Zev


\
 
 \ /
  Last update: 2008-08-27 06:33    [W:0.092 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site