linux-kernel - Re: [PATCH] tools/nolibc: align signature of ioctl() with POSIX

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] [day] [month] [year] [list]
Message-ID: <56babb03-9f16-4b5c-bd32-36446b5a56c7@t-8ch.de>
Date: Thu, 23 Jan 2025 17:50:19 +0100
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Willy Tarreau <w@....eu>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tools/nolibc: align signature of ioctl() with POSIX

On 2025-01-23 14:51:47+0100, Willy Tarreau wrote:
> On Thu, Jan 23, 2025 at 02:24:13PM +0100, Thomas Weißschuh wrote:
> > On 2025-01-23 14:16:25+0100, Willy Tarreau wrote:
> > > Hi Thomas,
> > > 
> > > On Thu, Jan 23, 2025 at 02:04:29PM +0100, Thomas Weißschuh wrote:
> > > > POSIX defines the signature of ioctl() as follows,
> > > > to allow passing a pointer or integer without casting:
> > > > 	int ioctl(int fildes, int request, ... /* arg */);
> > > > 
> > > > Nolibc ioctl() expects a pointer, forcing the user to manually cast.
> > > > Using va_arg to make the signature more flexible would work but seems to
> > > > prevent inlining of the function. Instead use a macro. "fd" and "req"
> > > > will still be typechecked through sys_ioctl().
> > > > 
> > > > Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> > > > ---
> > > >  tools/include/nolibc/sys.h | 8 ++------
> > > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> > > > index d4a5c2399a66b200ebf7ab249569cce2285481a5..5cb2c66cc8cccc4d4a1126acfd0b30a6efc886c3 100644
> > > > --- a/tools/include/nolibc/sys.h
> > > > +++ b/tools/include/nolibc/sys.h
> > > > @@ -532,7 +532,7 @@ uid_t getuid(void)
> > > >  
> > > >  
> > > >  /*
> > > > - * int ioctl(int fd, unsigned long req, void *value);
> > > > + * int ioctl(int fd, unsigned long req, ... value);
> > > >   */
> > > >  
> > > >  static __attribute__((unused))
> > > > @@ -541,11 +541,7 @@ int sys_ioctl(int fd, unsigned long req, void *value)
> > > >  	return my_syscall3(__NR_ioctl, fd, req, value);
> > > >  }
> > > >  
> > > > -static __attribute__((unused))
> > > > -int ioctl(int fd, unsigned long req, void *value)
> > > > -{
> > > > -	return __sysret(sys_ioctl(fd, req, value));
> > > > -}
> > > > +#define ioctl(fd, req, value) __sysret(sys_ioctl(fd, req, (void *)(value)))
> > > 
> > > You risk to get a warning about casting a pointer from an integer of
> > > a different size if you pass an int there. I think should should perform
> > > a double cast instead:
> > > 
> > >   #define ioctl(fd, req, value) __sysret(sys_ioctl(fd, req, (void *)(uintptr_t)(value)))
> > > 
> > > That way any int should cast fine, and pointers should as well.
> > 
> > I don't think this should ever happen. A warning there is actually
> > useful. The POSIX signature forces users to pass something that is
> > compatible with (void *), otherwise the vararg handling would be
> > invalid.
> 
> I'm a bit confused because while my man page says:
> 
>    The third argument is an untyped pointer to memory.
> 
> what I'm finding in POSIX is:
>  
>    The type of arg depends upon the particular control request, but it shall
>    be either an integer or a pointer to a device-specific data structure.
> 
> So I'm not seeing anything insisting on the int being the size of a
> pointer. Even the FreeBSD man explicitly mentions char *, caddr_t or
> int. Thus I do think that int is expressly permitted and might exist.

Normally ioctl() is implemented with va_arg. Inside ioctl() a type has
to be passed to va_arg(). This is a pointer/uintptr_t. In stdarg(3) the
following note is present for va_arg():

       If there is no next argument, or if type is  not  compatible  with  the
       type  of the actual next argument (as promoted according to the default
       argument promotions), random errors will occur.

This would be the case if an "int" argument is interpreted by va_arg(void *).
I can observe this with the test program below on arm64. va_list used
pointers internally and dereferencing an int pointer as long pointer
also yields random other bytes.

*But* the ioctl handler knows that the argument should have been an int
all along and should cast away the extra garbage bytes.

> I mean, I don't care much, but if we try to fix something that requires
> a cast, to end up with users having to write "(long)i" in their code,
> we won't have made much progress :-/

Instead of a double cast, we should change the type of sys_ioctl to
long sys_ioctl(unsigned int, unsigned int, unsigned long);
which is the actual syscall signature.
Then a single cast is enough for pointers and integers:
#define ioctl(fd, req, value) __sysret(sys_ioctl(fd, req, (unsigned long)(value)))


Test program:

#include <stdarg.h>
#include <stdio.h>

static void foo(int count, ...)
{
	unsigned long val;
	va_list ap;

	va_start(ap, count);
	for (int i = 0; i < count; i ++) {
		val = va_arg(ap, unsigned long);
		if (val != 0x05060708)
			printf("%d: 0x%lx\n", i, val);
	}
	va_end(ap);
}

int main(void)
{
	unsigned int vals[] = {
		0x01020304,
		0x05060708,
		0x090B0E01,
	};

	foo(20,
		vals[1], vals[1], vals[1], vals[1], vals[1],
		vals[1], vals[1], vals[1], vals[1], vals[1],
		vals[1], vals[1], vals[1], vals[1], vals[1],
		vals[1], vals[1], vals[1], vals[1], vals[1]
	);
}