* [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).