linux-kernel - Re: [PATCH] vsprintf: simplify number handling

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]

Message-ID: <CAHk-=whbzEO5sHk777FGWcCjDnX2QLBLX9XszEVh5GnSp+8RWw@mail.gmail.com>
Date: Wed, 18 Dec 2024 09:32:45 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, Daniel Borkmann <daniel@...earbox.net>, 
	Florent Revest <revest@...gle.com>, LKML <linux-kernel@...r.kernel.org>, 
	bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH] vsprintf: simplify number handling

On Wed, 18 Dec 2024 at 07:31, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> I went to test this by adding the following:
>
> diff --git a/samples/trace_printk/trace-printk.c b/samples/trace_printk/trace-printk.c
> index cfc159580263..1ff688637404 100644
> --- a/samples/trace_printk/trace-printk.c
> +++ b/samples/trace_printk/trace-printk.c
> @@ -43,6 +43,17 @@ static int __init trace_printk_init(void)
>
>         trace_printk(trace_printk_test_global_str_fmt, "", "dynamic string");
>
> +       trace_printk("Print unsigned long long %llu\n", -1LL);
> +       trace_printk("Print long long %lld\n", -1LL);
> +       trace_printk("Print unsigned long %llu\n", -1L);
> +       trace_printk("Print long  %ld\n", -1L);
> +       trace_printk("Print unsigned int %u\n", -1);
> +       trace_printk("Print int %d\n", -1);
> +       trace_printk("Print unsigned short %hu\n", (short)-1);
> +       trace_printk("Print short %hd\n", (short)-1);
> +       trace_printk("Print unsigned char %hhu\n", (char)-1);
> +       trace_printk("Print char %hhd\n", (char)-1);

For testing the real corner cases, you should probably check the real
truncation handling.

IOW, things like '%hh{d,u}' together with a value like 0x3456789ab.

Because yes, the truncation is very much part of the number handling,
and is actually a very important part of printk, and really the only
actual reason '%hhd' and friends even exist (without truncation, you'd
just use %d and %u).

The fact that both you and Rasmus felt that part needed more of a
comment clearly just means that I may be more aware of it than most
people are.

Because I considered the "handle sign" issue to be also making sure we
handle the size and the *lack* of sign corrrectly.

So I'll extend on the comment.

As you also point out with your tracing test:

>         modprobe-905   [003] .....   113.624842: bprint:               [FAILED TO PARSE] ip=0xffffffffc060e045 fmt=0xffff8c05c338e760 buf=ARRAY[]
>         modprobe-905   [003] .....   113.624843: bprint:               [FAILED TO PARSE] ip=0xffffffffc060e045 fmt=0xffff8c05c338ec40 buf=ARRAY[]
>         modprobe-905   [003] .....   113.624843: bprint:               [FAILED TO PARSE] ip=0xffffffffc060e045 fmt=0xffff8c05c338e280 buf=ARRAY[]
>
> Those "[FAILED TO PARSE]" messages have nothing to do with your code, but
> it means that it doesn't handle 'h' at all. Even the "unsigned short"
> printed but still failed to parse properly.

Yeah, %h{d,u} and %hh{d,u} are not hugely common, and apparently it's
not just your tracing tools that don't understand them: Alexei
reported that the bpf binary printk code also refused them.

That said, they *do* exist in the kernel, including in tracing:

    git grep 'TP_printk.*".*%hh*[ud].*"'

doesn't return lots of hits, but does report a handful.

> This is because libtraceevent appears to not support "%h" in print formats.
> That at least means there would be no breakage if they are modified in any
> way.

Oh, %hd is not getting modified (and if I did, that would be a major bug).

It's very much a part of the standard printf format, and is very much
inherent to the whole varargs and C integer promotion rules (ie you
literally *cannot* pass an actual 'char' value to a varargs function -
the normal C integer type extension rules apply).

So this is not really some odd kernel extension, and while there are
only a handful of users in tracing (that apparently trace-cmd cannot
deal with), it's not even _that_ uncommon in general:

    git grep '".*%hh*[ud].*"' | wc -l

reports that we have 501 of them in the kernel sources.

           Linus