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

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]

Date:   Tue, 22 Feb 2022 11:35:39 +0100
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Petr Mladek <pmladek@...e.com>,
        Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Miguel Ojeda <ojeda@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        rust-for-linux <rust-for-linux@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Gary Guo <gary@...yguo.net>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...gle.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCH v4 12/20] vsprintf: add new `%pA` format specifier

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@...musvillemoes.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