| Message ID | 1343636122-23273-1-git-send-email-tiejun.chen@windriver.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 07/30/2012 04:15 PM, Tiejun Chen wrote: > We miss that correct WDIOC_GETSUPPORT return path when perform > copy_to_user() properly. Any comments? Thanks Tiejun > > Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> > --- > drivers/watchdog/booke_wdt.c | 7 ++++--- > 1 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/watchdog/booke_wdt.c b/drivers/watchdog/booke_wdt.c > index 3fe82d0..2be7f29 100644 > --- a/drivers/watchdog/booke_wdt.c > +++ b/drivers/watchdog/booke_wdt.c > @@ -162,12 +162,13 @@ static long booke_wdt_ioctl(struct file *file, > unsigned int cmd, unsigned long arg) > { > u32 tmp = 0; > - u32 __user *p = (u32 __user *)arg; > + void __user *argp = (u32 __user *)arg; > + u32 __user *p = argp; > > switch (cmd) { > case WDIOC_GETSUPPORT: > - if (copy_to_user((void *)arg, &ident, sizeof(ident))) > - return -EFAULT; > + return copy_to_user(argp, &ident, > + sizeof(ident)) ? -EFAULT : 0; > case WDIOC_GETSTATUS: > return put_user(0, p); > case WDIOC_GETBOOTSTATUS: >
On Aug 5, 2012, at 10:10 PM, tiejun.chen wrote: > On 07/30/2012 04:15 PM, Tiejun Chen wrote: >> We miss that correct WDIOC_GETSUPPORT return path when perform >> copy_to_user() properly. > > Any comments? > > Thanks > Tiejun Adding Timur, as he's touched watchdog last. - k >> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> >> --- >> drivers/watchdog/booke_wdt.c | 7 ++++--- >> 1 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/watchdog/booke_wdt.c b/drivers/watchdog/booke_wdt.c >> index 3fe82d0..2be7f29 100644 >> --- a/drivers/watchdog/booke_wdt.c >> +++ b/drivers/watchdog/booke_wdt.c >> @@ -162,12 +162,13 @@ static long booke_wdt_ioctl(struct file *file, >> unsigned int cmd, unsigned long arg) >> { >> u32 tmp = 0; >> - u32 __user *p = (u32 __user *)arg; >> + void __user *argp = (u32 __user *)arg; >> + u32 __user *p = argp; >> >> switch (cmd) { >> case WDIOC_GETSUPPORT: >> - if (copy_to_user((void *)arg, &ident, sizeof(ident))) >> - return -EFAULT; >> + return copy_to_user(argp, &ident, >> + sizeof(ident)) ? -EFAULT : 0; >> case WDIOC_GETSTATUS: >> return put_user(0, p); >> case WDIOC_GETBOOTSTATUS: >>
On Mon, Jul 30, 2012 at 3:15 AM, Tiejun Chen <tiejun.chen@windriver.com> wrote: > We miss that correct WDIOC_GETSUPPORT return path when perform > copy_to_user() properly. Thanks for catching this. I'm amazed that this driver still has bugs like this. > diff --git a/drivers/watchdog/booke_wdt.c b/drivers/watchdog/booke_wdt.c > index 3fe82d0..2be7f29 100644 > --- a/drivers/watchdog/booke_wdt.c > +++ b/drivers/watchdog/booke_wdt.c > @@ -162,12 +162,13 @@ static long booke_wdt_ioctl(struct file *file, > unsigned int cmd, unsigned long arg) > { > u32 tmp = 0; > - u32 __user *p = (u32 __user *)arg; > + void __user *argp = (u32 __user *)arg; > + u32 __user *p = argp; You don't need to create 'argp'. The existing 'p' variable will work in the copy_to_user() call. > + return copy_to_user(argp, &ident, > + sizeof(ident)) ? -EFAULT : 0; This can fit in one line, especially if you use 'p' instead of 'argp'.
On Mon, Aug 6, 2012 at 2:12 PM, Tabi Timur-B04825 <b04825@freescale.com> wrote: > On Mon, Jul 30, 2012 at 3:15 AM, Tiejun Chen <tiejun.chen@windriver.com> wrote: >> We miss that correct WDIOC_GETSUPPORT return path when perform >> copy_to_user() properly. > > Thanks for catching this. I'm amazed that this driver still has bugs like this. While you're at it, I found a few related bugs. Can you fix these, also? 1. case WDIOC_SETOPTIONS: if (get_user(tmp, p)) return -EINVAL; This should return -EFAULT. 2. case WDIOC_GETBOOTSTATUS: /* XXX: something is clearing TSR */ tmp = mfspr(SPRN_TSR) & TSR_WRS(3); /* returns CARDRESET if last reset was caused by the WDT */ return (tmp ? WDIOF_CARDRESET : 0); This should use put_user() to return the value, instead of returning it as a return code. You can title the new patch something like, "booke/wdt: some ioctls do not return values properly"
On 08/07/2012 09:19 AM, Tabi Timur-B04825 wrote: > On Mon, Aug 6, 2012 at 2:12 PM, Tabi Timur-B04825 <b04825@freescale.com> wrote: >> On Mon, Jul 30, 2012 at 3:15 AM, Tiejun Chen <tiejun.chen@windriver.com> wrote: >>> We miss that correct WDIOC_GETSUPPORT return path when perform >>> copy_to_user() properly. >> >> Thanks for catching this. I'm amazed that this driver still has bugs like this. > > While you're at it, I found a few related bugs. Can you fix these, also? > > 1. case WDIOC_SETOPTIONS: > if (get_user(tmp, p)) > return -EINVAL; > > This should return -EFAULT. > > 2. case WDIOC_GETBOOTSTATUS: > /* XXX: something is clearing TSR */ > tmp = mfspr(SPRN_TSR) & TSR_WRS(3); > /* returns CARDRESET if last reset was caused by the WDT */ > return (tmp ? WDIOF_CARDRESET : 0); > > This should use put_user() to return the value, instead of returning > it as a return code. > > You can title the new patch something like, "booke/wdt: some ioctls do > not return values properly" Will regenerate this patch including these error as v2. Thanks Tiejun
diff --git a/drivers/watchdog/booke_wdt.c b/drivers/watchdog/booke_wdt.c index 3fe82d0..2be7f29 100644 --- a/drivers/watchdog/booke_wdt.c +++ b/drivers/watchdog/booke_wdt.c @@ -162,12 +162,13 @@ static long booke_wdt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { u32 tmp = 0; - u32 __user *p = (u32 __user *)arg; + void __user *argp = (u32 __user *)arg; + u32 __user *p = argp; switch (cmd) { case WDIOC_GETSUPPORT: - if (copy_to_user((void *)arg, &ident, sizeof(ident))) - return -EFAULT; + return copy_to_user(argp, &ident, + sizeof(ident)) ? -EFAULT : 0; case WDIOC_GETSTATUS: return put_user(0, p); case WDIOC_GETBOOTSTATUS:
We miss that correct WDIOC_GETSUPPORT return path when perform copy_to_user() properly. Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> --- drivers/watchdog/booke_wdt.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)