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 <[email protected]>
---
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
On Thu, Aug 4, 2022 at 9:06 AM Dan Carpenter <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
On Thu, Aug 04, 2022 at 10:20:34AM +0200, Arnd Bergmann wrote:
> On Thu, Aug 4, 2022 at 9:06 AM Dan Carpenter <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Thu, 4 Aug 2022 10:06:38 +0300
Dan Carpenter <[email protected]> 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
[email protected] on the ignore list for it.
Thanks
Alan
Alan asked to be added to the .get_maintainer.ignore list.
Signed-off-by: Dan Carpenter <[email protected]>
---
.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 <[email protected]>
+Alan Cox <[email protected]>
Christoph Hellwig <[email protected]>
Marc Gonzalez <[email protected]>
--
2.35.1