linux-kernel - Re: [PATCH] char: mwave: fix return type in ioctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220804083512.GK3460@kadam>
Date:   Thu, 4 Aug 2022 11:35:12 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alan Cox <alan@...rguk.ukuu.org.uk>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        kernel-janitors <kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH] char: mwave: fix return type in ioctl

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@...cle.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@...cle.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