Messages in this thread | | | Date | Tue, 22 Feb 2022 11:35:39 +0100 | Subject | Re: [PATCH v4 12/20] vsprintf: add new `%pA` format specifier | From | Rasmus Villemoes <> |
| |
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
| |