Re: [PATCH v4 12/20] vsprintf: add new `%pA` format specifier

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

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
>> <linux@rasmusvillemoes.dk> 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