Messages in this thread | | | Date | Thu, 24 Feb 2022 10:55:33 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH v4 12/20] vsprintf: add new `%pA` format specifier |
| |
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 > >> <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.
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
| |