Re: lib/58453: endptr can be unitialized if an invalid base is passed to strto*(3)

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Mon, 22 Jul 2024 20:13:51 -0400
    From:        Christos Zoulas <christos%zoulas.com@localhost>
    Message-ID:  <2984FE66-0D2E-41ED-A70E-0755EE1D1F46%zoulas.com@localhost>

  | Yes, when there is an invalid base. strtoi did not handle this properly
  | before the patch.

Actually, as I understand it, unless you're looking at the POSIX
definition of strtoimax(), there was no problem there on NetBSD,
because of how our strtoimax() behaves.

The actual problem that was fixed in the change cited related to
distinguishing between the function being cancelled and it simply
doing nothing (no number present I think might have been the alternative),
which I guess we could have a test for, but I certainly have no idea how
to write, and it isn't clear we ever had an actual issue there either.

  | > 3. christos@'s commit lifts this assumption so that the strtoi code we
  | >   use works in terms of either NetBSD's or glibc's
  | >   strtoimax/strtoumax.

Yes, though there's not a lot of reason for that, other than strict
correctness (no objections to dealing with that of course) - NetBSD's
strtoi() is very unlikely to ever call a glibc strtoimax().   Someone
else's copy of our strtoi() might of course.

If we wanted to be absolutely correct (not just our libc vs glibc)
and deal with other possible bizarre implementations of this issue,
it would need to be more complex, or coded differently.

  | > I asked christos@ to commit an ATF test for strtoi that exercises a
  | > path that, _under glibc's implementation_ of strtoimax/strtoumax,
  | > would use uninitialized memory.  That way, we have a chance -- e.g.,
  | > via ubsan, or just by initializing it to some garbage pointer into
  | > unmapped oblivion -- of detecting the nonportable assumption in strtoi
  | > in case we ever change our strtoimax/strtoumax implementation.

Since that assumption also got fixed, I believe, I doubt that a test
for that would be easy to create, and while this

  | I just added a test that checks that strtoimax returns an initialized
  | pointer

is reasonable, and harmless (verifying our implementation doesn't change),
it isn't what was requested.

Further, the actual undefined (unspecified) behaviour is what *entptr
gets set to (if anything at all) - not whether it points to valid memory.
Simply accessing *endptr is UB according to the POSIX (and I presume C)
definition of strtoimax() (and the other basic strtoX() functions).

That is, the implementation (of the strtoimax() etc functions) is permitted
to place absolutely anything in the *endptr passed to it, if the base is
invalid, including a pointer that will cause a trap if referenced (not
just dereferenced).   I'm not aware of any of our supported ports where
such a pointer value exists though.  ubsan (about which I know < zero)
might possibly be able to detect such an undefined use however.

I thought the changes to strtoi() were such that it dealt correctly
with this possibility (even though it never occurs on NetBSD) but on
a closer examination I'm not sure that's true.

A simpler change to strtoi() to deal with all of this UB stuff would be to
validate the base passed to it, rather than relying upon strtoimax()
doing that - either by duplicating the code, or by all relevant functions
calling an internal (ie _xxx()) function to do it, so they all remain
consistent - that could be a static inline function from some #ifdef _LIBC
section of some (internal) header file, so that the function could be
defined just once, but avoid the function call overhead for the
relatively trivial test that is needed.

At least now, I think, there's enough information in this PR that
some sense can be made of it, though I'm not sure that there's anything
much required to handle it, and I doubt the ECANCELED change is really
important enough to bother with a pullup, the other change certainly
isn't.   After all, Alejandro's message containing the patch did say:

alx%kernel.org@localhost said:
  | I'm not sure if this patch is wanted in NetBSD.  It doesn't fix any bugs
  | there. 

Fixing it in HEAD makes sense however:

alx%kernel.org@localhost said:
  | for those pasting this code in other systems (which is currently done by
  | libbsd).

but we don't need it pulled up for that.

kre