Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <10bbffc2-f144-8555-d41b-fede69a13c16@rasmusvillemoes.dk>
Date:   Tue, 22 Feb 2022 11:35:39 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v4 12/20] vsprintf: add new `%pA` format specifier
Content-Language: en-US
To:     Petr Mladek <pmladek@suse.com>,
        Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Miguel Ojeda <ojeda@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        rust-for-linux <rust-for-linux@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Gary Guo <gary@garyguo.net>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@google.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Sergey Senozhatsky <senozhatsky@chromium.org>
References: <20220212130410.6901-1-ojeda@kernel.org>
 <20220212130410.6901-13-ojeda@kernel.org>
 <YgosclY9ebD3t020@smile.fi.intel.com>
 <dc9054f2-5e2b-0ae2-1022-23421668dd05@rasmusvillemoes.dk>
 <CANiq72=mNh415UG_nxDQc=9SCSVSPnqcAbgFZKzavnmsim_L0Q@mail.gmail.com>
 <YhSs5ZTL9ixdCCU9@alley>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
In-Reply-To: <YhSs5ZTL9ixdCCU9@alley>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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