* [PATCH 1/7] 68328serial: check return value of copy_*_user() instead of access_ok()
@ 2010-07-31 17:38 Kulikov Vasiliy
2010-07-31 19:09 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Kulikov Vasiliy @ 2010-07-31 17:38 UTC (permalink / raw
To: kernel-janitors
Cc: Greg Kroah-Hartman, Andrew Morton, Greg Ungerer, Christoph Egger,
Tejun Heo, linux-kernel
As copy_*_user() calls access_ok() it should not be called explicitly.
Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
---
drivers/serial/68328serial.c | 29 ++++++++++-------------------
1 files changed, 10 insertions(+), 19 deletions(-)
diff --git a/drivers/serial/68328serial.c b/drivers/serial/68328serial.c
index 7356a56..ab6ad8b 100644
--- a/drivers/serial/68328serial.c
+++ b/drivers/serial/68328serial.c
@@ -869,7 +869,9 @@ static int get_serial_info(struct m68k_serial * info,
tmp.close_delay = info->close_delay;
tmp.closing_wait = info->closing_wait;
tmp.custom_divisor = info->custom_divisor;
- copy_to_user(retinfo,&tmp,sizeof(*retinfo));
+ if (copy_to_user(retinfo, &tmp, sizeof(*retinfo)))
+ return -EFAULT;
+
return 0;
}
@@ -882,7 +884,8 @@ static int set_serial_info(struct m68k_serial * info,
if (!new_info)
return -EFAULT;
- copy_from_user(&new_serial,new_info,sizeof(new_serial));
+ if (copy_from_user(&new_serial, new_info, sizeof(new_serial)))
+ return -EFAULT;
old_info = *info;
if (!capable(CAP_SYS_ADMIN)) {
@@ -943,8 +946,7 @@ static int get_lsr_info(struct m68k_serial * info, unsigned int *value)
status = 0;
#endif
local_irq_restore(flags);
- put_user(status,value);
- return 0;
+ return put_user(status, value);
}
/*
@@ -999,27 +1001,16 @@ static int rs_ioctl(struct tty_struct *tty, struct file * file,
send_break(info, arg ? arg*(100) : 250);
return 0;
case TIOCGSERIAL:
- if (access_ok(VERIFY_WRITE, (void *) arg,
- sizeof(struct serial_struct)))
- return get_serial_info(info,
- (struct serial_struct *) arg);
- return -EFAULT;
+ return get_serial_info(info,
+ (struct serial_struct *) arg);
case TIOCSSERIAL:
return set_serial_info(info,
(struct serial_struct *) arg);
case TIOCSERGETLSR: /* Get line status register */
- if (access_ok(VERIFY_WRITE, (void *) arg,
- sizeof(unsigned int)))
- return get_lsr_info(info, (unsigned int *) arg);
- return -EFAULT;
+ return get_lsr_info(info, (unsigned int *) arg);
case TIOCSERGSTRUCT:
- if (!access_ok(VERIFY_WRITE, (void *) arg,
- sizeof(struct m68k_serial)))
- return -EFAULT;
- copy_to_user((struct m68k_serial *) arg,
+ return copy_to_user((struct m68k_serial *) arg,
info, sizeof(struct m68k_serial));
- return 0;
-
default:
return -ENOIOCTLCMD;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/7] 68328serial: check return value of copy_*_user() instead of access_ok()
2010-07-31 17:38 [PATCH 1/7] 68328serial: check return value of copy_*_user() instead of access_ok() Kulikov Vasiliy
@ 2010-07-31 19:09 ` Dan Carpenter
2010-08-01 6:14 ` Vasiliy Kulikov
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2010-07-31 19:09 UTC (permalink / raw
To: Kulikov Vasiliy
Cc: kernel-janitors, Greg Kroah-Hartman, Andrew Morton, Greg Ungerer,
Christoph Egger, Tejun Heo, linux-kernel
On Sat, Jul 31, 2010 at 09:38:00PM +0400, Kulikov Vasiliy wrote:
> - sizeof(struct m68k_serial)))
> - return -EFAULT;
> - copy_to_user((struct m68k_serial *) arg,
> + return copy_to_user((struct m68k_serial *) arg,
> info, sizeof(struct m68k_serial));
We should return if -EFAULT copy_to_user() failes here.
> - return 0;
> -
> default:
> return -ENOIOCTLCMD;
> }
Smatch would have caught that but I don't have a cross compile
environment set up.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/7] 68328serial: check return value of copy_*_user() instead of access_ok()
2010-07-31 19:09 ` Dan Carpenter
@ 2010-08-01 6:14 ` Vasiliy Kulikov
0 siblings, 0 replies; 3+ messages in thread
From: Vasiliy Kulikov @ 2010-08-01 6:14 UTC (permalink / raw
To: Dan Carpenter, kernel-janitors, Greg Kroah-Hartman, Andrew Morton,
Greg Ungerer, Christoph Egger, Tejun Heo, linux-kernel
On Sat, Jul 31, 2010 at 21:09 +0200, Dan Carpenter wrote:
> On Sat, Jul 31, 2010 at 09:38:00PM +0400, Kulikov Vasiliy wrote:
> > - sizeof(struct m68k_serial)))
> > - return -EFAULT;
> > - copy_to_user((struct m68k_serial *) arg,
> > + return copy_to_user((struct m68k_serial *) arg,
> > info, sizeof(struct m68k_serial));
>
> We should return if -EFAULT copy_to_user() failes here.
Right, I was looking for this bug and made it myself :)
Thank you.
>
> > - return 0;
> > -
> > default:
> > return -ENOIOCTLCMD;
> > }
>
> Smatch would have caught that but I don't have a cross compile
> environment set up.
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-08-01 6:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-07-31 17:38 [PATCH 1/7] 68328serial: check return value of copy_*_user() instead of access_ok() Kulikov Vasiliy
2010-07-31 19:09 ` Dan Carpenter
2010-08-01 6:14 ` Vasiliy Kulikov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).