Re: [PATCH 1/1] booke/wdt: fix incorrect WDIOC_GETSUPPORT return path
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.c...@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:
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/1] booke/wdt: fix incorrect WDIOC_GETSUPPORT return path
On Mon, Jul 30, 2012 at 3:15 AM, Tiejun Chen tiejun.c...@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'.
--
Timur Tabi
Linux kernel developer at Freescale
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/1] booke/wdt: fix incorrect WDIOC_GETSUPPORT return path
On Mon, Aug 6, 2012 at 2:12 PM, Tabi Timur-B04825 b04...@freescale.com wrote: On Mon, Jul 30, 2012 at 3:15 AM, Tiejun Chen tiejun.c...@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 -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/1] booke/wdt: fix incorrect WDIOC_GETSUPPORT return path
On 08/07/2012 09:19 AM, Tabi Timur-B04825 wrote: On Mon, Aug 6, 2012 at 2:12 PM, Tabi Timur-B04825 b04...@freescale.com wrote: On Mon, Jul 30, 2012 at 3:15 AM, Tiejun Chen tiejun.c...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/1] booke/wdt: fix incorrect WDIOC_GETSUPPORT return path
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.c...@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:
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
