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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
