[musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[Ralph Little]

Hi,
I have been picking up some old pending issues related to the SANE project.
One of our CI builds is on Alpine and it is generating warnings for 
ioctl() calls from the musl library:

|error: overflow in conversion from 'long unsigned int' to 'int' changes 
value from '2147577985' to '-2147389311' [-Werror=overflow]
|
||ioctl (fd, PPRSTATUS, &status);

||I see that Olaf Meeuwissen raised this issue a couple of years ago and 
the discussion petered out somewhat and I don't believe that the issue 
was ever really resolved:

https://www.openwall.com/lists/musl/2020/01/20/2

Is there any possibility that this could be addressed in the near future?
I see that Alpine have closed their issue and are not interested in 
patching their downstream musl:

https://gitlab.alpinelinux.org/alpine/aports/-/issues/7580#note_287168

Cheers,
Ralph Little

Re: [musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[Markus Wichmann]

On Mon, Feb 20, 2023 at 09:26:05PM -0800, Ralph Little wrote:
> Hi,
> I have been picking up some old pending issues related to the SANE project.
> One of our CI builds is on Alpine and it is generating warnings for ioctl()
> calls from the musl library:
>
> |error: overflow in conversion from 'long unsigned int' to 'int' changes
> value from '2147577985' to '-2147389311' [-Werror=overflow]
> |
> ||ioctl (fd, PPRSTATUS, &status);
>
> ||I see that Olaf Meeuwissen raised this issue a couple of years ago and the
> discussion petered out somewhat and I don't believe that the issue was ever
> really resolved:
>
> https://www.openwall.com/lists/musl/2020/01/20/2
>
> Is there any possibility that this could be addressed in the near future?
> I see that Alpine have closed their issue and are not interested in patching
> their downstream musl:
>
> https://gitlab.alpinelinux.org/alpine/aports/-/issues/7580#note_287168
>
> Cheers,
> Ralph Little
>
>
>
>
>

So, I had a look at it. As far as I can tell, the issue is that musl
declares ioctl()'s second argument to be an int. Together with the other
defintions, this means that any _IOC_READ constant will overflow and
generate those warnings. Also, this is technically undefined behavior,
as value bits are shifted into the sign bit of a signed integer.

Linux itself defines the ioctl syscall to have a second argument of type
unsigned int.

So this issue could be resolved by simply making the second argument of
the ioctl() function unsigned. Does that create ABI issues? To my
knowledge, all ABIs pass ints and unsigned ints the same way. Even if on
some 64-bit arch there was a sign extension at the top, only the low
32 bits are defined.

Ciao,
Markus

Re: [musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[Florian Weimer]

* Markus Wichmann:

> So this issue could be resolved by simply making the second argument of
> the ioctl() function unsigned. Does that create ABI issues?

POSIX mandates the int type.

Thanks,
Florian

Re: [musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[alice]

On Tue Feb 21, 2023 at 5:42 PM CET, Florian Weimer wrote:
> * Markus Wichmann:
>
> > So this issue could be resolved by simply making the second argument of
> > the ioctl() function unsigned. Does that create ABI issues?
>
> POSIX mandates the int type.

it does, but as it has been mentioned, how is one meant to use the _IOC*
thingies then without leading to UB?

at the end of the day, the libc is on linux only, and so if linux does not
use "int" itself for the api, ...

>
> Thanks,
> Florian

Re: [musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[Jeffrey Walton]

On Tue, Feb 21, 2023 at 11:05 AM Markus Wichmann <nullplan@gmx.net> wrote:
> On Mon, Feb 20, 2023 at 09:26:05PM -0800, Ralph Little wrote:
> > I have been picking up some old pending issues related to the SANE project.
> > One of our CI builds is on Alpine and it is generating warnings for ioctl()
> > calls from the musl library:
> >
> > |error: overflow in conversion from 'long unsigned int' to 'int' changes
> > value from '2147577985' to '-2147389311' [-Werror=overflow]
> > |
> > ||ioctl (fd, PPRSTATUS, &status);
> >
> > ||I see that Olaf Meeuwissen raised this issue a couple of years ago and the
> > discussion petered out somewhat and I don't believe that the issue was ever
> > really resolved:
> >
> > https://www.openwall.com/lists/musl/2020/01/20/2
> >
> > Is there any possibility that this could be addressed in the near future?
> > I see that Alpine have closed their issue and are not interested in patching
> > their downstream musl:
> >
> > https://gitlab.alpinelinux.org/alpine/aports/-/issues/7580#note_287168
> [...]
> So, I had a look at it. As far as I can tell, the issue is that musl
> declares ioctl()'s second argument to be an int. Together with the other
> defintions, this means that any _IOC_READ constant will overflow and
> generate those warnings. Also, this is technically undefined behavior,
> as value bits are shifted into the sign bit of a signed integer.
>
> Linux itself defines the ioctl syscall to have a second argument of type
> unsigned int.
>
> So this issue could be resolved by simply making the second argument of
> the ioctl() function unsigned. Does that create ABI issues? To my
> knowledge, all ABIs pass ints and unsigned ints the same way. Even if on
> some 64-bit arch there was a sign extension at the top, only the low
> 32 bits are defined.

In this case, I think the best course of action is to cast a,b,c to
unsigned, then perform the shifts, and finally cast back to int. That
is what the C standard requires. And it should not mess with the ABI.

If the code remains undefined, then it is subject to removal by the
compiler. The casts, while ugly, keep the code in well defined
territory. Also, if anyone ever performs testing with
-fsantize=undefined, then the code will trigger real findings that
could keep the code from passing through a security gate (for those
folks who have to work in that kind of environment).

I've had to work bug reports that were a result of the missing casts
during shifts and rotates. It is not fun. I was able to track all of
them down with -fsantize=undefined .

Jeff

Re: [musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[Ralph Little]

[-- Attachment #1: Type: text/plain, Size: 3089 bytes --]

Hi,

On Tue, Feb 21, 2023 at 8:58 AM Jeffrey Walton <noloader@gmail.com> wrote:

> On Tue, Feb 21, 2023 at 11:05 AM Markus Wichmann <nullplan@gmx.net> wrote:
> > On Mon, Feb 20, 2023 at 09:26:05PM -0800, Ralph Little wrote:
> > > I have been picking up some old pending issues related to the SANE
> project.
> > > One of our CI builds is on Alpine and it is generating warnings for
> ioctl()
> > > calls from the musl library:
> > >
> > > |error: overflow in conversion from 'long unsigned int' to 'int'
> changes
> > > value from '2147577985' to '-2147389311' [-Werror=overflow]
> > > |
> > > ||ioctl (fd, PPRSTATUS, &status);
> > >
> > > ||I see that Olaf Meeuwissen raised this issue a couple of years ago
> and the
> > > discussion petered out somewhat and I don't believe that the issue was
> ever
> > > really resolved:
> > >
> > > https://www.openwall.com/lists/musl/2020/01/20/2
> > >
> > > Is there any possibility that this could be addressed in the near
> future?
> > > I see that Alpine have closed their issue and are not interested in
> patching
> > > their downstream musl:
> > >
> > > https://gitlab.alpinelinux.org/alpine/aports/-/issues/7580#note_287168
> > [...]
> > So, I had a look at it. As far as I can tell, the issue is that musl
> > declares ioctl()'s second argument to be an int. Together with the other
> > defintions, this means that any _IOC_READ constant will overflow and
> > generate those warnings. Also, this is technically undefined behavior,
> > as value bits are shifted into the sign bit of a signed integer.
> >
> > Linux itself defines the ioctl syscall to have a second argument of type
> > unsigned int.
> >
> > So this issue could be resolved by simply making the second argument of
> > the ioctl() function unsigned. Does that create ABI issues? To my
> > knowledge, all ABIs pass ints and unsigned ints the same way. Even if on
> > some 64-bit arch there was a sign extension at the top, only the low
> > 32 bits are defined.
>
> In this case, I think the best course of action is to cast a,b,c to
> unsigned, then perform the shifts, and finally cast back to int. That
> is what the C standard requires. And it should not mess with the ABI.
>
> If the code remains undefined, then it is subject to removal by the
> compiler. The casts, while ugly, keep the code in well defined
> territory. Also, if anyone ever performs testing with
> -fsantize=undefined, then the code will trigger real findings that
> could keep the code from passing through a security gate (for those
> folks who have to work in that kind of environment).
>
> I've had to work bug reports that were a result of the missing casts
> during shifts and rotates. It is not fun. I was able to track all of
> them down with -fsantize=undefined .
>
> Jeff
>

That would be cool by me. I hate making macros complicated but I also
understand the POSIX issue.
Seems a shame that POSIX is so out of step these days with actual
implementations. Perhaps that will change in the future....

Cool to be reminded of the GCC santize settings. I had forgotten about
those.

Cheers,
Ralph

[-- Attachment #2: Type: text/html, Size: 4131 bytes --]

Re: [musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[Markus Wichmann]

On Tue, Feb 21, 2023 at 11:57:50AM -0500, Jeffrey Walton wrote:
> In this case, I think the best course of action is to cast a,b,c to
> unsigned, then perform the shifts, and finally cast back to int. That
> is what the C standard requires. And it should not mess with the ABI.
>

Actually I messed up. That mostly happens already. The directions are
defined as unsigned, and the conversion to int happens implicitly. So
there is no undefined behavior. The compiler is merely warning about
nothing. That also means the only thing missing to silence the warning
would be a cast back to int. However, we have in the past never added
code just to shut up overeager warnings. Should the compiler not
suppress warnings that come from system headers?

Ciao,
Markus

Re: [musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[NRK]

On Tue, Feb 21, 2023 at 10:28:42PM +0100, Markus Wichmann wrote:
> Should the compiler not suppress warnings that come from system
> headers?

As far as I see, the warning isn't coming "from system headers". The
header is only defining those constants, the user is the one who's
using that constant in an `int` context.

This isn't much different than the user trying to put `ULONG_MAX` into
an `char` for example. And I don't see how the compiler can "fix" it
unless you introduce some sort of "this constant is OK to be converted
to an int" attribute (which would still require musl having to put the
attribute in from their side).

> Linux itself defines the ioctl syscall to have a second argument of
> type unsigned int.

My local ioctl(2) manpage says it's unsigned _long_ (not `int`).

- NRK

Re: [musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[Rich Felker]

On Tue, Feb 21, 2023 at 05:04:55PM +0100, Markus Wichmann wrote:
> On Mon, Feb 20, 2023 at 09:26:05PM -0800, Ralph Little wrote:
> > Hi,
> > I have been picking up some old pending issues related to the SANE project.
> > One of our CI builds is on Alpine and it is generating warnings for ioctl()
> > calls from the musl library:
> >
> > |error: overflow in conversion from 'long unsigned int' to 'int' changes
> > value from '2147577985' to '-2147389311' [-Werror=overflow]
> > |
> > ||ioctl (fd, PPRSTATUS, &status);
> >
> > ||I see that Olaf Meeuwissen raised this issue a couple of years ago and the
> > discussion petered out somewhat and I don't believe that the issue was ever
> > really resolved:
> >
> > https://www.openwall.com/lists/musl/2020/01/20/2
> >
> > Is there any possibility that this could be addressed in the near future?
> > I see that Alpine have closed their issue and are not interested in patching
> > their downstream musl:
> >
> > https://gitlab.alpinelinux.org/alpine/aports/-/issues/7580#note_287168
> >
> > Cheers,
> > Ralph Little
> >
> >
> >
> >
> >
> 
> So, I had a look at it. As far as I can tell, the issue is that musl
> declares ioctl()'s second argument to be an int. Together with the other
> defintions, this means that any _IOC_READ constant will overflow and
> generate those warnings. Also, this is technically undefined behavior,
> as value bits are shifted into the sign bit of a signed integer.

Unless you're seeing something I'm not, there's no UB. The shifts take
place on the unsigned type, and the conversion from unsigned to signed
is implementation-defined, not undefined. The implementation-defined
definition relevant to us is modular reduction.

I'm not sure if there's anything reasonable that can be done here on
our side to be more friendly while still conformant, but getting rid
of -Werror=overflow (which is treating well-defined code as an error)
will solve the problem.

Rich

Re: [musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[Markus Wichmann]

On Tue, Feb 21, 2023 at 10:17:31PM -0500, Rich Felker wrote:
> Unless you're seeing something I'm not, there's no UB. The shifts take
> place on the unsigned type, and the conversion from unsigned to signed
> is implementation-defined, not undefined. The implementation-defined
> definition relevant to us is modular reduction.
>

Yeah, sorry, I had missed that the directions are defined as unsigned
constants. That turns any shift that might have been undefined into an
unsigned shift, where it is defined. As I said in the other mail, the
compiler is just warning about nothing here, and it should probably not
warn for system header files, anyway.

We may be able to silence the compiler by adding an explicit int
conversion to the _IOC macro. But I'm not sure if we want to set a
precedent that we will add code just to shut up overeager warnings.

Ciao,
Markus

Re: [musl] Re: [BUG] ioctl: overflow in implicit constant conversion

[Rich Felker]

On Wed, Feb 22, 2023 at 05:23:12AM +0100, Markus Wichmann wrote:
> On Tue, Feb 21, 2023 at 10:17:31PM -0500, Rich Felker wrote:
> > Unless you're seeing something I'm not, there's no UB. The shifts take
> > place on the unsigned type, and the conversion from unsigned to signed
> > is implementation-defined, not undefined. The implementation-defined
> > definition relevant to us is modular reduction.
> >
> 
> Yeah, sorry, I had missed that the directions are defined as unsigned
> constants. That turns any shift that might have been undefined into an
> unsigned shift, where it is defined. As I said in the other mail, the
> compiler is just warning about nothing here, and it should probably not
> warn for system header files, anyway.
> 
> We may be able to silence the compiler by adding an explicit int
> conversion to the _IOC macro. But I'm not sure if we want to set a
> precedent that we will add code just to shut up overeager warnings.

I think it's reasonably arguable that the ioctl command macros should
have the right type matching the type the ioctl function takes. But
it's possible there could be places where sign extension bites you
then. As a stupid example, if someone bypassed the libc function and
tried to make the ioctl syscall directly, having the macros be of
signed type would cause the kernel to receive them with a bunch of
ones in the high bits, and possibly reject them as a result. I'm not
sure if there's any reasonable usage that would be impacted, though.

Rich