From: Gary Guo <[email protected]>
This patch adds a format specifier `%pA` to `vsprintf` which formats
a pointer as `core::fmt::Arguments`. Doing so allows us to directly
format to the internal buffer of `printf`, so we do not have to use
a temporary buffer on the stack to pre-assemble the message on
the Rust side.
This specifier is intended only to be used from Rust and not for C, so
`checkpatch.pl` is intentionally unchanged to catch any misuse.
Co-developed-by: Alex Gaynor <[email protected]>
Signed-off-by: Alex Gaynor <[email protected]>
Co-developed-by: Wedson Almeida Filho <[email protected]>
Signed-off-by: Wedson Almeida Filho <[email protected]>
Signed-off-by: Gary Guo <[email protected]>
Co-developed-by: Miguel Ojeda <[email protected]>
Signed-off-by: Miguel Ojeda <[email protected]>
---
lib/rust.h | 14 ++++++++++++++
lib/vsprintf.c | 7 +++++++
2 files changed, 21 insertions(+)
create mode 100644 lib/rust.h
diff --git a/lib/rust.h b/lib/rust.h
new file mode 100644
index 000000000000..9cf0b102b496
--- /dev/null
+++ b/lib/rust.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LIB_RUST_H
+#define _LIB_RUST_H
+
+#ifdef CONFIG_RUST
+char *rust_fmt_argument(char* buf, char* end, void *ptr);
+#else
+static inline char *rust_fmt_argument(char* buf, char* end, void *ptr)
+{
+ return NULL;
+}
+#endif
+
+#endif /* _LIB_RUST_H */
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b8129dd374c..61528094ec87 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -52,6 +52,7 @@
#include <linux/string_helpers.h>
#include "kstrtox.h"
+#include "rust.h"
static noinline unsigned long long simple_strntoull(const char *startp, size_t max_chars, char **endp, unsigned int base)
{
@@ -2378,6 +2379,10 @@ early_param("no_hash_pointers", no_hash_pointers_enable);
*
* Note: The default behaviour (unadorned %p) is to hash the address,
* rendering it useful as a unique identifier.
+ *
+ * There is also a '%pA' format specifier, but it is only intended to be used
+ * from Rust code to format core::fmt::Arguments. Do *not* use it from C.
+ * See rust/kernel/print.rs for details.
*/
static noinline_for_stack
char *pointer(const char *fmt, char *buf, char *end, void *ptr,
@@ -2450,6 +2455,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
return device_node_string(buf, end, ptr, spec, fmt + 1);
case 'f':
return fwnode_string(buf, end, ptr, spec, fmt + 1);
+ case 'A':
+ return rust_fmt_argument(buf, end, ptr);
case 'x':
return pointer_string(buf, end, ptr, spec);
case 'e':
--
2.35.1
On Sat, Feb 12, 2022 at 02:03:38PM +0100, Miguel Ojeda wrote:
> From: Gary Guo <[email protected]>
Not sure I understand this...
> This patch adds a format specifier `%pA` to `vsprintf` which formats
> a pointer as `core::fmt::Arguments`. Doing so allows us to directly
> format to the internal buffer of `printf`, so we do not have to use
> a temporary buffer on the stack to pre-assemble the message on
> the Rust side.
>
> This specifier is intended only to be used from Rust and not for C, so
> `checkpatch.pl` is intentionally unchanged to catch any misuse.
>
> Co-developed-by: Alex Gaynor <[email protected]>
> Signed-off-by: Alex Gaynor <[email protected]>
> Co-developed-by: Wedson Almeida Filho <[email protected]>
> Signed-off-by: Wedson Almeida Filho <[email protected]>
> Signed-off-by: Gary Guo <[email protected]>
...together with this in the current SoB chain.
> Co-developed-by: Miguel Ojeda <[email protected]>
> Signed-off-by: Miguel Ojeda <[email protected]>
I'm wondering if you considered to use %pV.
--
With Best Regards,
Andy Shevchenko
On 14/02/2022 11.18, Andy Shevchenko wrote:
> On Sat, Feb 12, 2022 at 02:03:38PM +0100, Miguel Ojeda wrote:
>
>> From: Gary Guo <[email protected]>
>
> Not sure I understand this...
>
>> This patch adds a format specifier `%pA` to `vsprintf` which formats
>> a pointer as `core::fmt::Arguments`. Doing so allows us to directly
>> format to the internal buffer of `printf`, so we do not have to use
>> a temporary buffer on the stack to pre-assemble the message on
>> the Rust side.
>>
>> This specifier is intended only to be used from Rust and not for C, so
>> `checkpatch.pl` is intentionally unchanged to catch any misuse.
>>
>> Co-developed-by: Alex Gaynor <[email protected]>
>> Signed-off-by: Alex Gaynor <[email protected]>
>> Co-developed-by: Wedson Almeida Filho <[email protected]>
>> Signed-off-by: Wedson Almeida Filho <[email protected]>
>
>> Signed-off-by: Gary Guo <[email protected]>
>
> ...together with this in the current SoB chain.
>
>> Co-developed-by: Miguel Ojeda <[email protected]>
>> Signed-off-by: Miguel Ojeda <[email protected]>
>
> I'm wondering if you considered to use %pV.
>
I think the point is for vsnprintf() to call (back) into Rust code.
That said, I don't like the !CONFIG_RUST version to return NULL, that
will surely crash moments later.
So I prefer something like
[rust.h]
// no CONFIG_RUST conditional
+char *rust_fmt_argument(char* buf, char* end, void *ptr);
[vsprintf.c]
+ case 'A':
+ if (IS_ENABLED(CONFIG_RUST))
+ return rust_fmt_argument(buf, end, ptr);
+ else
+ return string_nocheck(buf, end, "[%pA in non-Rust
code?!]", default_str_spec);
Rasmus
From: Rasmus Villemoes
> Sent: 14 February 2022 10:53
>
> On 14/02/2022 11.18, Andy Shevchenko wrote:
> > On Sat, Feb 12, 2022 at 02:03:38PM +0100, Miguel Ojeda wrote:
> >
> >> From: Gary Guo <[email protected]>
> >
> > Not sure I understand this...
> >
> >> This patch adds a format specifier `%pA` to `vsprintf` which formats
> >> a pointer as `core::fmt::Arguments`. Doing so allows us to directly
> >> format to the internal buffer of `printf`, so we do not have to use
> >> a temporary buffer on the stack to pre-assemble the message on
> >> the Rust side.
> >>
> >> This specifier is intended only to be used from Rust and not for C, so
> >> `checkpatch.pl` is intentionally unchanged to catch any misuse.
> >>
> >> Co-developed-by: Alex Gaynor <[email protected]>
> >> Signed-off-by: Alex Gaynor <[email protected]>
> >> Co-developed-by: Wedson Almeida Filho <[email protected]>
> >> Signed-off-by: Wedson Almeida Filho <[email protected]>
> >
> >> Signed-off-by: Gary Guo <[email protected]>
> >
> > ...together with this in the current SoB chain.
> >
> >> Co-developed-by: Miguel Ojeda <[email protected]>
> >> Signed-off-by: Miguel Ojeda <[email protected]>
> >
> > I'm wondering if you considered to use %pV.
> >
>
> I think the point is for vsnprintf() to call (back) into Rust code.
Doesn't that stand a reasonable chance of blowing the kernel stack?
vsnprintf() is likely to be on the 'worst case' stack path anyway.
Anything vaguely like a recursive call, or anything 'stack expensive'
inside vsnprintf() stands a real chance of overflowing the stack.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, Feb 14, 2022 at 01:27:48PM +0100, Miguel Ojeda wrote:
> On Mon, Feb 14, 2022 at 11:19 AM Andy Shevchenko
> <[email protected]> wrote:
> >
> > Not sure I understand this...
> >
> > ...together with this in the current SoB chain.
>
> From my reading of `Documentation/process/submitting-patches.rst`,
> this is the case I have to use:
>
> ```
> Example of a patch submitted by a Co-developed-by: author::
>
> From: From Author <[email protected]>
>
> <changelog>
>
> Co-developed-by: Random Co-Author <[email protected]>
> Signed-off-by: Random Co-Author <[email protected]>
> Signed-off-by: From Author <[email protected]>
> Co-developed-by: Submitting Co-Author <[email protected]>
> Signed-off-by: Submitting Co-Author <[email protected]>
> ```
>
> Do you think another case applies?
I see, thanks for elaboration!
...
> > I'm wondering if you considered to use %pV.
>
> Please see Rasmus' reply, i.e. we are using Rust's own formatting
> machinery (the compiler validates the format string and creates an
> object that represents the formatting to be done).
Yes, I read that.
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 14, 2022 at 11:52 AM Rasmus Villemoes
<[email protected]> wrote:
>
> I think the point is for vsnprintf() to call (back) into Rust code.
Indeed, this is the case.
> That said, I don't like the !CONFIG_RUST version to return NULL, that
> will surely crash moments later.
>
> So I prefer something like
>
> [rust.h]
> // no CONFIG_RUST conditional
> +char *rust_fmt_argument(char* buf, char* end, void *ptr);
>
> [vsprintf.c]
> + case 'A':
> + if (IS_ENABLED(CONFIG_RUST))
> + return rust_fmt_argument(buf, end, ptr);
> + else
> + return string_nocheck(buf, end, "[%pA in non-Rust
> code?!]", default_str_spec);
Sounds good. Or perhaps simply `break` and let it print the pointer
(to be consistent with `g` case and non-error `e` case).
Cheers,
Miguel
On Mon, Feb 14, 2022 at 12:37 PM David Laight <[email protected]> wrote:
>
> Doesn't that stand a reasonable chance of blowing the kernel stack?
>
> vsnprintf() is likely to be on the 'worst case' stack path anyway.
> Anything vaguely like a recursive call, or anything 'stack expensive'
> inside vsnprintf() stands a real chance of overflowing the stack.
There should be no recursion going on, but stack usage could be a
concern depending on what the formatted types attempt to do (thus we
need to be mindful when implementing `Display`) and what the compiler
could possibly generate on a worst case (Gary, bjorn3 et. al. may want
to give details about this).
Cheers,
Miguel
On Mon, Feb 14, 2022 at 2:55 PM Andy Shevchenko
<[email protected]> wrote:
>
> I see, thanks for elaboration!
Anytime!
Cheers,
Miguel
Hi Andy,
On Mon, Feb 14, 2022 at 11:19 AM Andy Shevchenko
<[email protected]> wrote:
>
> Not sure I understand this...
>
> ...together with this in the current SoB chain.
From my reading of `Documentation/process/submitting-patches.rst`,
this is the case I have to use:
```
Example of a patch submitted by a Co-developed-by: author::
From: From Author <[email protected]>
<changelog>
Co-developed-by: Random Co-Author <[email protected]>
Signed-off-by: Random Co-Author <[email protected]>
Signed-off-by: From Author <[email protected]>
Co-developed-by: Submitting Co-Author <[email protected]>
Signed-off-by: Submitting Co-Author <[email protected]>
```
Do you think another case applies?
> I'm wondering if you considered to use %pV.
Please see Rasmus' reply, i.e. we are using Rust's own formatting
machinery (the compiler validates the format string and creates an
object that represents the formatting to be done).
Thanks for taking a look!
Cheers,
Miguel
On 22/02/2022 10.29, Petr Mladek wrote:
> On Mon 2022-02-14 13:12:24, Miguel Ojeda wrote:
>> On Mon, Feb 14, 2022 at 11:52 AM Rasmus Villemoes
>> <[email protected]> wrote:
>>>
>>> I think the point is for vsnprintf() to call (back) into Rust code.
>>
>> Indeed, this is the case.
>>
>>> That said, I don't like the !CONFIG_RUST version to return NULL, that
>>> will surely crash moments later.
>>>
>>> So I prefer something like
>>>
>>> [rust.h]
>>> // no CONFIG_RUST conditional
>>> +char *rust_fmt_argument(char* buf, char* end, void *ptr);
>>>
>>> [vsprintf.c]
>>> + case 'A':
>>> + if (IS_ENABLED(CONFIG_RUST))
>>> + return rust_fmt_argument(buf, end, ptr);
>>> + else
>>> + return string_nocheck(buf, end, "[%pA in non-Rust
>>> code?!]", default_str_spec);
>
> Any long message might cause buffer overflow when the caller expects
> fixed short string.
If the caller (1) uses a %p extension from C code which should only be
used from Rust and (2) uses sprintf() or another variant where he
doesn't provide the real buffer bounds, well, then he certainly gets to
keep the pieces.
It is a much worse problem that if CONFIG_RUST is enabled, we can't know
that we were actually called from Rust (but when !CONFIG_RUST, we
certainly know that we weren't), so we could call into rust_fmt_argument
with a pointer which certainly doesn't point to the/a data structure
which that Rust code expects. But we can't do anything about it, we will
just have to rely on static analysis to flag any use of %pA in C code.
> The most safe solution would be to use WARN_ONCE().
Preferably no, we shouldn't call into the printk machinery from within
vsnprintf(). I know I've added a few myself (AFAIR for use of %n or
other unsupported specifiers, and for overflow of precision/field
width), and I've often thought about a way to get rid of them while
still making sure some message eventually gets logged (once).
Rasmus
On Mon 2022-02-14 13:12:24, Miguel Ojeda wrote:
> On Mon, Feb 14, 2022 at 11:52 AM Rasmus Villemoes
> <[email protected]> wrote:
> >
> > I think the point is for vsnprintf() to call (back) into Rust code.
>
> Indeed, this is the case.
>
> > That said, I don't like the !CONFIG_RUST version to return NULL, that
> > will surely crash moments later.
> >
> > So I prefer something like
> >
> > [rust.h]
> > // no CONFIG_RUST conditional
> > +char *rust_fmt_argument(char* buf, char* end, void *ptr);
> >
> > [vsprintf.c]
> > + case 'A':
> > + if (IS_ENABLED(CONFIG_RUST))
> > + return rust_fmt_argument(buf, end, ptr);
> > + else
> > + return string_nocheck(buf, end, "[%pA in non-Rust
> > code?!]", default_str_spec);
Any long message might cause buffer overflow when the caller expects
fixed short string.
> Sounds good. Or perhaps simply `break` and let it print the pointer
> (to be consistent with `g` case and non-error `e` case).
Also this might cause buffer overflow.
The most safe solution would be to use WARN_ONCE(). The only drawback
is that it might cause panic() when using "panic_on_warn" kernel
parameter. But it will not open security hole.
Best Regards,
Petr
On Tue 2022-02-22 11:35:39, Rasmus Villemoes wrote:
> On 22/02/2022 10.29, Petr Mladek wrote:
> > On Mon 2022-02-14 13:12:24, Miguel Ojeda wrote:
> >> On Mon, Feb 14, 2022 at 11:52 AM Rasmus Villemoes
> >> <[email protected]> wrote:
> >>>
> >>> I think the point is for vsnprintf() to call (back) into Rust code.
> >>
> >> Indeed, this is the case.
> >>
> >>> That said, I don't like the !CONFIG_RUST version to return NULL, that
> >>> will surely crash moments later.
> >>>
> >>> So I prefer something like
> >>>
> >>> [rust.h]
> >>> // no CONFIG_RUST conditional
> >>> +char *rust_fmt_argument(char* buf, char* end, void *ptr);
> >>>
> >>> [vsprintf.c]
> >>> + case 'A':
> >>> + if (IS_ENABLED(CONFIG_RUST))
> >>> + return rust_fmt_argument(buf, end, ptr);
> >>> + else
> >>> + return string_nocheck(buf, end, "[%pA in non-Rust
> >>> code?!]", default_str_spec);
> >
> > Any long message might cause buffer overflow when the caller expects
> > fixed short string.
>
> If the caller (1) uses a %p extension from C code which should only be
> used from Rust and (2) uses sprintf() or another variant where he
> doesn't provide the real buffer bounds, well, then he certainly gets to
> keep the pieces.
>
> It is a much worse problem that if CONFIG_RUST is enabled, we can't know
> that we were actually called from Rust (but when !CONFIG_RUST, we
> certainly know that we weren't), so we could call into rust_fmt_argument
> with a pointer which certainly doesn't point to the/a data structure
> which that Rust code expects. But we can't do anything about it, we will
> just have to rely on static analysis to flag any use of %pA in C code.
Yeah. !CONFIG_RUST would trigger the warning and help to find the
sinners but it is not reliable. Static analysic might be better...
> > The most safe solution would be to use WARN_ONCE().
>
> Preferably no, we shouldn't call into the printk machinery from within
> vsnprintf(). I know I've added a few myself (AFAIR for use of %n or
> other unsupported specifiers, and for overflow of precision/field
> width), and I've often thought about a way to get rid of them while
> still making sure some message eventually gets logged (once).
WARN_ONCE() in vsprintf() code is much more acceptable these days
with the lockless ringbuffer.
Best Regards,
Petr