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

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

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

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>

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