[musl] [PATCH] ioctl: Fix implicit constant conversion overflow

[musl] [PATCH] ioctl: Fix implicit constant conversion overflow

[Ismael Luceno]

The last parameter (result of sizeof) to _IOC in _IOR/_IOW/_IOWR causes
the underlying expression's value to be promoted to size_t. Casting it
to int resolves the issue.

Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
---
 arch/generic/bits/ioctl.h   | 6 +++---
 arch/mips/bits/ioctl.h      | 6 +++---
 arch/mipsn32/bits/ioctl.h   | 6 +++---
 arch/powerpc/bits/ioctl.h   | 6 +++---
 arch/powerpc64/bits/ioctl.h | 6 +++---
 arch/sh/bits/ioctl.h        | 6 +++---
 6 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/generic/bits/ioctl.h b/arch/generic/bits/ioctl.h
index 60ae8b850b17..16541d547f68 100644
--- a/arch/generic/bits/ioctl.h
+++ b/arch/generic/bits/ioctl.h
@@ -4,9 +4,9 @@
 #define _IOC_READ  2U
 
 #define _IO(a,b) _IOC(_IOC_NONE,(a),(b),0)
-#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),sizeof(c))
-#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),sizeof(c))
-#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),sizeof(c))
+#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),(int)sizeof(c))
+#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),(int)sizeof(c))
+#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),(int)sizeof(c))
 
 #define TCGETS		0x5401
 #define TCSETS		0x5402
diff --git a/arch/mips/bits/ioctl.h b/arch/mips/bits/ioctl.h
index e20bf19eaa77..e165bb8588a2 100644
--- a/arch/mips/bits/ioctl.h
+++ b/arch/mips/bits/ioctl.h
@@ -4,9 +4,9 @@
 #define _IOC_WRITE 4U
 
 #define _IO(a,b) _IOC(_IOC_NONE,(a),(b),0)
-#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),sizeof(c))
-#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),sizeof(c))
-#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),sizeof(c))
+#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),(int)sizeof(c))
+#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),(int)sizeof(c))
+#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),(int)sizeof(c))
 
 #define TCGETA		0x5401
 #define TCSETA		0x5402
diff --git a/arch/mipsn32/bits/ioctl.h b/arch/mipsn32/bits/ioctl.h
index e20bf19eaa77..e165bb8588a2 100644
--- a/arch/mipsn32/bits/ioctl.h
+++ b/arch/mipsn32/bits/ioctl.h
@@ -4,9 +4,9 @@
 #define _IOC_WRITE 4U
 
 #define _IO(a,b) _IOC(_IOC_NONE,(a),(b),0)
-#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),sizeof(c))
-#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),sizeof(c))
-#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),sizeof(c))
+#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),(int)sizeof(c))
+#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),(int)sizeof(c))
+#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),(int)sizeof(c))
 
 #define TCGETA		0x5401
 #define TCSETA		0x5402
diff --git a/arch/powerpc/bits/ioctl.h b/arch/powerpc/bits/ioctl.h
index ac9bfd204a84..a9f45f321abc 100644
--- a/arch/powerpc/bits/ioctl.h
+++ b/arch/powerpc/bits/ioctl.h
@@ -4,9 +4,9 @@
 #define _IOC_READ  2U
 
 #define _IO(a,b) _IOC(_IOC_NONE,(a),(b),0)
-#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),sizeof(c))
-#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),sizeof(c))
-#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),sizeof(c))
+#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),(int)sizeof(c))
+#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),(int)sizeof(c))
+#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),(int)sizeof(c))
 
 #define FIONCLEX	_IO('f', 2)
 #define FIOCLEX		_IO('f', 1)
diff --git a/arch/powerpc64/bits/ioctl.h b/arch/powerpc64/bits/ioctl.h
index b6cbb18f4776..425d96a44444 100644
--- a/arch/powerpc64/bits/ioctl.h
+++ b/arch/powerpc64/bits/ioctl.h
@@ -4,9 +4,9 @@
 #define _IOC_READ  2U
 
 #define _IO(a,b) _IOC(_IOC_NONE,(a),(b),0)
-#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),sizeof(c))
-#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),sizeof(c))
-#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),sizeof(c))
+#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),(int)sizeof(c))
+#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),(int)sizeof(c))
+#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),(int)sizeof(c))
 
 #define FIONCLEX	_IO('f', 2)
 #define FIOCLEX		_IO('f', 1)
diff --git a/arch/sh/bits/ioctl.h b/arch/sh/bits/ioctl.h
index 370b6901e261..7d423e861c52 100644
--- a/arch/sh/bits/ioctl.h
+++ b/arch/sh/bits/ioctl.h
@@ -4,9 +4,9 @@
 #define _IOC_READ  2U
 
 #define _IO(a,b) _IOC(_IOC_NONE,(a),(b),0)
-#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),sizeof(c))
-#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),sizeof(c))
-#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),sizeof(c))
+#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),(int)sizeof(c))
+#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),(int)sizeof(c))
+#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),(int)sizeof(c))
 
 #define FIOCLEX             _IO('f',  1)
 #define FIONCLEX            _IO('f',  2)
-- 
2.44.0

Re: [musl] [PATCH] ioctl: Fix implicit constant conversion overflow

[Rich Felker]

On Sat, Jun 01, 2024 at 03:03:25AM +0200, Ismael Luceno wrote:
> The last parameter (result of sizeof) to _IOC in _IOR/_IOW/_IOWR causes
> the underlying expression's value to be promoted to size_t. Casting it
> to int resolves the issue.
> 
> Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> ---
>  arch/generic/bits/ioctl.h   | 6 +++---
>  arch/mips/bits/ioctl.h      | 6 +++---
>  arch/mipsn32/bits/ioctl.h   | 6 +++---
>  arch/powerpc/bits/ioctl.h   | 6 +++---
>  arch/powerpc64/bits/ioctl.h | 6 +++---
>  arch/sh/bits/ioctl.h        | 6 +++---
>  6 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/generic/bits/ioctl.h b/arch/generic/bits/ioctl.h
> index 60ae8b850b17..16541d547f68 100644
> --- a/arch/generic/bits/ioctl.h
> +++ b/arch/generic/bits/ioctl.h
> @@ -4,9 +4,9 @@
>  #define _IOC_READ  2U
>  
>  #define _IO(a,b) _IOC(_IOC_NONE,(a),(b),0)
> -#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),sizeof(c))
> -#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),sizeof(c))
> -#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),sizeof(c))
> +#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),(int)sizeof(c))
> +#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),(int)sizeof(c))
> +#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),(int)sizeof(c))

I don't see how this helps with the warning you're trying to suppress,
since _IOC_{READ,WRITE} already have unsigned type. If you changed
that, you would then have *real overflows* (undefined behavior)
instead of the well-defined, valid implicit conversions -Werror is
complaining about.

There may be a way to improve on the situation here but it's not so
simple.

Rich

Re: [musl] [PATCH] ioctl: Fix implicit constant conversion overflow

[Ismael Luceno]

On 31/May/2024 22:34, Rich Felker wrote:
<...>
> > +#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),(int)sizeof(c))
> > +#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),(int)sizeof(c))
> > +#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),(int)sizeof(c))
> 
> I don't see how this helps with the warning you're trying to suppress,

GCC disagrees; the warnings go away because it's this element that
causes the whole expression to be promoted to unsigned long long,
so making it smaller (we can use unsigned int instead) avoids the
issue.

> since _IOC_{READ,WRITE} already have unsigned type. If you changed
> that, you would then have *real overflows* (undefined behavior)
> instead of the well-defined, valid implicit conversions -Werror is
> complaining about.

This expresssion is supposed to fit 32-bit anyway, isn't it?

Re: [musl] [PATCH] ioctl: Fix implicit constant conversion overflow

[Rich Felker]

On Sun, Jun 02, 2024 at 05:01:10AM +0200, Ismael Luceno wrote:
> On 31/May/2024 22:34, Rich Felker wrote:
> <...>
> > > +#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),(int)sizeof(c))
> > > +#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),(int)sizeof(c))
> > > +#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),(int)sizeof(c))
> > 
> > I don't see how this helps with the warning you're trying to suppress,
> 
> GCC disagrees; the warnings go away because it's this element that
> causes the whole expression to be promoted to unsigned long long,
> so making it smaller (we can use unsigned int instead) avoids the
> issue.

In that case gcc is just being inconsistent. Both the conversion from
unsigned int to int and size_t to int are non-value-preserving. It
makes no sense that it warns for the latter but not for the former.

"Make weird inconsistent warning messages go away" is not a motivation
for a change. If the command macros could all be made to have type int
(matcing the ioctl argument) without introducing new problems, that
would be a well-motivated change. I suppose "make them have type
unsigned int rather than unsigned long so that they're not
gratuitously over-wide" might be well-motivated too, but I suspect it
leaves in place warnings in some places. "Fix implicit constant
conversion overflow" is not a well-motivated change since there is no
overflow.

Rich

Re: [musl] [PATCH] ioctl: Fix implicit constant conversion overflow

[Ismael Luceno]

On 02/Jun/2024 18:50, Rich Felker wrote:
> On Sun, Jun 02, 2024 at 05:01:10AM +0200, Ismael Luceno wrote:
> > On 31/May/2024 22:34, Rich Felker wrote:
> > <...>
> > > > +#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),(int)sizeof(c))
> > > > +#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),(int)sizeof(c))
> > > > +#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),(int)sizeof(c))
> > > 
> > > I don't see how this helps with the warning you're trying to suppress,
> > 
> > GCC disagrees; the warnings go away because it's this element that
> > causes the whole expression to be promoted to unsigned long long,
> > so making it smaller (we can use unsigned int instead) avoids the
> > issue.
> 
> In that case gcc is just being inconsistent. Both the conversion from
> unsigned int to int and size_t to int are non-value-preserving. It
> makes no sense that it warns for the latter but not for the former.
> 
> "Make weird inconsistent warning messages go away" is not a motivation
> for a change. If the command macros could all be made to have type int
> (matcing the ioctl argument) without introducing new problems, that
> would be a well-motivated change. I suppose "make them have type
> unsigned int rather than unsigned long so that they're not
> gratuitously over-wide" might be well-motivated too, but I suspect it
> leaves in place warnings in some places. "Fix implicit constant
> conversion overflow" is not a well-motivated change since there is no
> overflow.

GCC doesn't make much sense here but the warning appears with several
versions of GCC.

An explicit cast at _IOC instead would make sense to me, but what could
break in your opinion?

Re: [musl] [PATCH] ioctl: Fix implicit constant conversion overflow

[Rich Felker]

On Mon, Jun 03, 2024 at 03:57:18AM +0200, Ismael Luceno wrote:
> On 02/Jun/2024 18:50, Rich Felker wrote:
> > On Sun, Jun 02, 2024 at 05:01:10AM +0200, Ismael Luceno wrote:
> > > On 31/May/2024 22:34, Rich Felker wrote:
> > > <...>
> > > > > +#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),(int)sizeof(c))
> > > > > +#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),(int)sizeof(c))
> > > > > +#define _IOWR(a,b,c) _IOC(_IOC_READ|_IOC_WRITE,(a),(b),(int)sizeof(c))
> > > > 
> > > > I don't see how this helps with the warning you're trying to suppress,
> > > 
> > > GCC disagrees; the warnings go away because it's this element that
> > > causes the whole expression to be promoted to unsigned long long,
> > > so making it smaller (we can use unsigned int instead) avoids the
> > > issue.
> > 
> > In that case gcc is just being inconsistent. Both the conversion from
> > unsigned int to int and size_t to int are non-value-preserving. It
> > makes no sense that it warns for the latter but not for the former.
> > 
> > "Make weird inconsistent warning messages go away" is not a motivation
> > for a change. If the command macros could all be made to have type int
> > (matcing the ioctl argument) without introducing new problems, that
> > would be a well-motivated change. I suppose "make them have type
> > unsigned int rather than unsigned long so that they're not
> > gratuitously over-wide" might be well-motivated too, but I suspect it
> > leaves in place warnings in some places. "Fix implicit constant
> > conversion overflow" is not a well-motivated change since there is no
> > overflow.
> 
> GCC doesn't make much sense here but the warning appears with several
> versions of GCC.
> 
> An explicit cast at _IOC instead would make sense to me, but what could
> break in your opinion?

I'm not sure. It needs investigation. There might have been some
concern with breakage from kernel headers that define ioctl numbers or
something. I just remember this hasn't been as simple as it sounds
from past times it came up..

Rich

Re: [musl] [PATCH] ioctl: Fix implicit constant conversion overflow

[Ismael Luceno]

On 10/Jun/2024 12:04, Rich Felker wrote:
> On Mon, Jun 03, 2024 at 03:57:18AM +0200, Ismael Luceno wrote:
<...>
> > An explicit cast at _IOC instead would make sense to me, but what
> > could break in your opinion?
> 
> I'm not sure. It needs investigation. There might have been some
> concern with breakage from kernel headers that define ioctl numbers
> or something. I just remember this hasn't been as simple as it
> sounds from past times it came up..

The following headers use the _IOC macro:
	asm/zcrypt.h (on S390)
	asm-generic/ioctls.h
	linux/hiddev.h
	linux/hidraw.h
	linux/input.h
	linux/joystick.h
	linux/uinput.h
	linux/usbdevice_fs.h
	linux/vboxguest.h
	sound/sb16_csp.h
	xen/evtchn.h
	xen/gntalloc.h
	xen/gntdev.h
	xen/privcmd.h

None of these seem to pose a problem.

The only problem I see is if someone tried to cast it to a larger
signed type, which would be definitely a bug in their code, otherwise,
casting it to any unsigned type and back shoulld be fine.

Probably POSIX is wrong here, and it should be uint32_t for better
compatibility...

Thoughts?