linux-kernel - Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing functions

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87jz577vk9.fsf@kernel.org>
Date: Thu, 19 Jun 2025 14:41:26 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <lossin@...nel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Alex Gaynor"
 <alex.gaynor@...il.com>,  "Boqun Feng" <boqun.feng@...il.com>,  "Gary Guo"
 <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Alice
 Ryhl" <aliceryhl@...gle.com>,  "Masahiro Yamada" <masahiroy@...nel.org>,
  "Nathan Chancellor" <nathan@...nel.org>,  "Luis Chamberlain"
 <mcgrof@...nel.org>,  "Danilo Krummrich" <dakr@...nel.org>,  "Nicolas
 Schier" <nicolas.schier@...ux.dev>,  "Trevor Gross" <tmgross@...ch.edu>,
  "Adam Bratschi-Kaye" <ark.email@...il.com>,
  <rust-for-linux@...r.kernel.org>,  <linux-kernel@...r.kernel.org>,
  <linux-kbuild@...r.kernel.org>,  "Petr Pavlu" <petr.pavlu@...e.com>,
  "Sami Tolvanen" <samitolvanen@...gle.com>,  "Daniel Gomez"
 <da.gomez@...sung.com>,  "Simona Vetter" <simona.vetter@...ll.ch>,  "Greg
 KH" <gregkh@...uxfoundation.org>,  "Fiona Behrens" <me@...enk.dev>,
  "Daniel Almeida" <daniel.almeida@...labora.com>,
  <linux-modules@...r.kernel.org>
Subject: Re: [PATCH v13 1/6] rust: str: add radix prefixed integer parsing
 functions

"Benno Lossin" <lossin@...nel.org> writes:

> On Thu Jun 19, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>> I'm having a difficult time parsing. Are you suggesting that we guard
>> against implementations of `TryInto<u64>` that misbehave?
>
> Let me try a different explanation:
>
> The safety requirement for implementing the `FromStrRadix`:
>
>     /// The member functions of this trait must be implemented according to
>     /// their documentation.
>
> Together with the functions of the trait:
>
>     /// Parse `src` to [`Self`] using radix `radix`.
>     fn from_str_radix(src: &BStr, radix: u32) -> Result<Self, crate::error::Error>;
>
>     /// Return the absolute value of [`Self::MIN`].
>     fn abs_min() -> u64;
>
>     /// Perform bitwise 2's complement on `self`.
>     ///
>     /// Note: This function does not make sense for unsigned integers.
>     fn complement(self) -> Self;
>
> Doesn't make sense. What does it mean to return the "absolute value of
> [`Self::MIN`]"? We don't have "absolute value" defined for an arbitrary
> type. Similarly for `complement` and `from_str_radix`, what does "Parse
> `src` to [`Self`] using radex `radix`" mean? It's not well-defined.
>
> You use this safety requirement in the parsing branch for negative
> numbers (the `unsafe` call at the bottom):
>
>     [b'-', rest @ ..] => {
>         let (radix, digits) = strip_radix(rest.as_ref());
>         // 2's complement values range from -2^(b-1) to 2^(b-1)-1.
>         // So if we want to parse negative numbers as positive and
>         // later multiply by -1, we have to parse into a larger
>         // integer. We choose `u64` as sufficiently large.
>         //
>         // NOTE: 128 bit integers are not available on all
>         // platforms, hence the choice of 64 bits.
>         let val =
>             u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
>                 .map_err(|_| EINVAL)?;
>
>         if val > Self::abs_min() {
>             return Err(EINVAL);
>         }
>
>         if val == Self::abs_min() {
>             return Ok(Self::MIN);
>         }
>
>         // SAFETY: We checked that `val` will fit in `Self` above.
>         let val: Self = unsafe { val.try_into().unwrap_unchecked() };
>
>         Ok(val.complement())
>     }
>
> But you don't mention that the check is valid due to the safety
> requirements of implementing `FromStrRadix`. But even if you did, that
> wouldn't mean anything as I explained above.
>
> So let's instead move all of this negation & u64 conversion logic into
> the `FromStrRadix` trait. Then it can be safe & the `ParseInt::from_str`
> function doesn't use `unsafe` (there still will be `unsafe` in the
> macro, but that is fine, as it's more local and knows the concrete
> types).
>

Alright. I guess my safety comments are slightly hand-wavy. Thanks for
the suggestion, I'll apply that for next spin.

Best regards,
Andreas Hindborg