* [PATCH] char: mwave: fix return type in ioctl @ 2022-08-04 7:06 Dan Carpenter 2022-08-04 8:20 ` Arnd Bergmann 2022-08-04 18:19 ` Alan 0 siblings, 2 replies; 5+ messages in thread From: Dan Carpenter @ 2022-08-04 7:06 UTC (permalink / raw) To: Arnd Bergmann Cc: Greg Kroah-Hartman, Andrew Morton, Alan Cox, linux-kernel, kernel-janitors This function is supposed to return zero for success or negative error code on failure. Unfortunately the "retval" is declared as unsigned int and the function returns type long. That means that on 64 bit systems it will return positive values on error. Fixes: 909d145f0dec ("mwave: ioctl BKL pushdown") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- The Fixes tag is sort of debatable. "retval" should have always been declared as an int. But the BKL change is when the return type for the ioctl changed from int to long, so it's when the bug started to affect user space. drivers/char/mwave/mwavedd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/mwave/mwavedd.c b/drivers/char/mwave/mwavedd.c index 11272d605ecd..5813517faf4d 100644 --- a/drivers/char/mwave/mwavedd.c +++ b/drivers/char/mwave/mwavedd.c @@ -123,7 +123,7 @@ static int mwave_close(struct inode *inode, struct file *file) static long mwave_ioctl(struct file *file, unsigned int iocmd, unsigned long ioarg) { - unsigned int retval = 0; + int retval = 0; pMWAVE_DEVICE_DATA pDrvData = &mwave_s_mdd; void __user *arg = (void __user *)ioarg; -- 2.35.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] char: mwave: fix return type in ioctl 2022-08-04 7:06 [PATCH] char: mwave: fix return type in ioctl Dan Carpenter @ 2022-08-04 8:20 ` Arnd Bergmann 2022-08-04 8:35 ` Dan Carpenter 2022-08-04 18:19 ` Alan 1 sibling, 1 reply; 5+ messages in thread From: Arnd Bergmann @ 2022-08-04 8:20 UTC (permalink / raw) To: Dan Carpenter Cc: Arnd Bergmann, Greg Kroah-Hartman, Andrew Morton, Alan Cox, Linux Kernel Mailing List, kernel-janitors On Thu, Aug 4, 2022 at 9:06 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > This function is supposed to return zero for success or negative error > code on failure. Unfortunately the "retval" is declared as unsigned int > and the function returns type long. That means that on 64 bit systems > it will return positive values on error. > > Fixes: 909d145f0dec ("mwave: ioctl BKL pushdown") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > The Fixes tag is sort of debatable. "retval" should have always been > declared as an int. But the BKL change is when the return type for > the ioctl changed from int to long, so it's when the bug started to > affect user space. Nice catch, I wonder how many other drivers I broke in that series. Have you gone through my BKL commits from that time period to see if any others are affected? Acked-by: Arnd Bergmann <arnd@arndb.de> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] char: mwave: fix return type in ioctl 2022-08-04 8:20 ` Arnd Bergmann @ 2022-08-04 8:35 ` Dan Carpenter 0 siblings, 0 replies; 5+ messages in thread From: Dan Carpenter @ 2022-08-04 8:35 UTC (permalink / raw) To: Arnd Bergmann Cc: Greg Kroah-Hartman, Andrew Morton, Alan Cox, Linux Kernel Mailing List, kernel-janitors On Thu, Aug 04, 2022 at 10:20:34AM +0200, Arnd Bergmann wrote: > On Thu, Aug 4, 2022 at 9:06 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > This function is supposed to return zero for success or negative error > > code on failure. Unfortunately the "retval" is declared as unsigned int > > and the function returns type long. That means that on 64 bit systems > > it will return positive values on error. > > > > Fixes: 909d145f0dec ("mwave: ioctl BKL pushdown") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > The Fixes tag is sort of debatable. "retval" should have always been > > declared as an int. But the BKL change is when the return type for > > the ioctl changed from int to long, so it's when the bug started to > > affect user space. > > Nice catch, I wonder how many other drivers I broke in that series. > Have you gone through my BKL commits from that time period > to see if any others are affected? Btw, I meant to thank you for your other email about IRQs. Thanks! Yeah. This is from static analysis. There aren't many other bugs. It's a combination of storing error codes in unsigned int and returning signed long. The first is kind of a bug even when it doesn't affect runtime and the second is not a bug but it's sort of rare. The one thing that might amuse you as history buff is a preserved bug for API reasons: arch/x86/kernel/ldt.c 665 SYSCALL_DEFINE3(modify_ldt, int , func , void __user * , ptr , 666 unsigned long , bytecount) 667 { 668 int ret = -ENOSYS; 669 670 switch (func) { 671 case 0: 672 ret = read_ldt(ptr, bytecount); 673 break; 674 case 1: 675 ret = write_ldt(ptr, bytecount, 1); 676 break; 677 case 2: 678 ret = read_default_ldt(ptr, bytecount); 679 break; 680 case 0x11: 681 ret = write_ldt(ptr, bytecount, 0); 682 break; 683 } 684 /* 685 * The SYSCALL_DEFINE() macros give us an 'unsigned long' 686 * return type, but tht ABI for sys_modify_ldt() expects 687 * 'int'. This cast gives us an int-sized value in %rax 688 * for the return code. The 'unsigned' is necessary so 689 * the compiler does not try to sign-extend the negative 690 * return codes into the high half of the register when 691 * taking the value from int->long. 692 */ 693 return (unsigned int)ret; 694 } regards, dan carpenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] char: mwave: fix return type in ioctl 2022-08-04 7:06 [PATCH] char: mwave: fix return type in ioctl Dan Carpenter 2022-08-04 8:20 ` Arnd Bergmann @ 2022-08-04 18:19 ` Alan 2022-08-10 9:18 ` [PATCH] get_maintainer: Add Alan to .get_maintainer.ignore Dan Carpenter 1 sibling, 1 reply; 5+ messages in thread From: Alan @ 2022-08-04 18:19 UTC (permalink / raw) To: Dan Carpenter Cc: Arnd Bergmann, Greg Kroah-Hartman, Andrew Morton, Alan Cox, linux-kernel, kernel-janitors On Thu, 4 Aug 2022 10:06:38 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > This function is supposed to return zero for success or negative error > code on failure. Unfortunately the "retval" is declared as unsigned int > and the function returns type long. That means that on 64 bit systems > it will return positive values on error. Not sure what tool folks are using these days to figure out who to mail about Linux bits, but could whoever maintains it stick alan@lxorguk.ukuu.org.uk on the ignore list for it. Thanks Alan ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] get_maintainer: Add Alan to .get_maintainer.ignore 2022-08-04 18:19 ` Alan @ 2022-08-10 9:18 ` Dan Carpenter 0 siblings, 0 replies; 5+ messages in thread From: Dan Carpenter @ 2022-08-10 9:18 UTC (permalink / raw) To: Andrew Morton, Alan Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, kernel-janitors Alan asked to be added to the .get_maintainer.ignore list. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- .get_maintainer.ignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.get_maintainer.ignore b/.get_maintainer.ignore index a64d21913745..c298bab3d320 100644 --- a/.get_maintainer.ignore +++ b/.get_maintainer.ignore @@ -1,2 +1,4 @@ +Alan Cox <alan@lxorguk.ukuu.org.uk> +Alan Cox <root@hraefn.swansea.linux.org.uk> Christoph Hellwig <hch@lst.de> Marc Gonzalez <marc.w.gonzalez@free.fr> -- 2.35.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-10 9:19 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-04 7:06 [PATCH] char: mwave: fix return type in ioctl Dan Carpenter 2022-08-04 8:20 ` Arnd Bergmann 2022-08-04 8:35 ` Dan Carpenter 2022-08-04 18:19 ` Alan 2022-08-10 9:18 ` [PATCH] get_maintainer: Add Alan to .get_maintainer.ignore Dan Carpenter
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 NNTP newsgroup(s).