Our formatted output helpers are currently implemented with
snprintf-like functions which take arguments as va_list but the types
stored in a va_list need to be known at compilation time which causes
problems when dealing with arguments from the BPF world that are always
u64 but considered differently depending on the format specifiers they
are associated with at runtime.
This series replaces snprintf usages with bstr_printf calls. This lets
us construct a binary representation of arguments in bpf_printf_prepare
at runtime that matches an ABI that is neither arch nor compiler
specific.
This solves a bug reported by Rasmus Villemoes that would mangle
arguments on 32 bit machines.
Florent Revest (2):
seq_file: Add a seq_bprintf function
bpf: Implement formatted output helpers with bstr_printf
fs/seq_file.c | 18 ++++
include/linux/bpf.h | 22 +----
include/linux/seq_file.h | 4 +
init/Kconfig | 1 +
kernel/bpf/helpers.c | 188 +++++++++++++++++++++------------------
kernel/bpf/verifier.c | 2 +-
kernel/trace/bpf_trace.c | 34 +++----
7 files changed, 137 insertions(+), 132 deletions(-)
--
2.31.1.498.g6c1eba8ee3d-goog
On 23/04/2021 03.15, Florent Revest wrote:
> Our formatted output helpers are currently implemented with
> snprintf-like functions which take arguments as va_list but the types
> stored in a va_list need to be known at compilation time which causes
> problems when dealing with arguments from the BPF world that are always
> u64 but considered differently depending on the format specifiers they
> are associated with at runtime.
>
> This series replaces snprintf usages with bstr_printf calls. This lets
> us construct a binary representation of arguments in bpf_printf_prepare
> at runtime that matches an ABI that is neither arch nor compiler
> specific.
>
> This solves a bug reported by Rasmus Villemoes that would mangle
> arguments on 32 bit machines.
That's not entirely accurate. The arguments are also mangled on x86-64,
it's just that in a few cases that goes unnoticed. That's why I
suggested you try and take your test case (which I assume had been
passing with flying colours on x86-64) and rearrange the specifiers,
arguments and expected output string so that the (morally) 32 bit
arguments end up beyond those-that-end-up-in-the-reg_save_area.
IOWs, it is the 32 bit arguments that are mangled (because they get
passed as-if they were actually 64 bits), and that applies on all
architectures; nothing to do with sizeof(long).
Rasmus
On Fri, Apr 23, 2021 at 10:50 AM Rasmus Villemoes
<[email protected]> wrote:
>
> On 23/04/2021 03.15, Florent Revest wrote:
> > Our formatted output helpers are currently implemented with
> > snprintf-like functions which take arguments as va_list but the types
> > stored in a va_list need to be known at compilation time which causes
> > problems when dealing with arguments from the BPF world that are always
> > u64 but considered differently depending on the format specifiers they
> > are associated with at runtime.
> >
> > This series replaces snprintf usages with bstr_printf calls. This lets
> > us construct a binary representation of arguments in bpf_printf_prepare
> > at runtime that matches an ABI that is neither arch nor compiler
> > specific.
> >
> > This solves a bug reported by Rasmus Villemoes that would mangle
> > arguments on 32 bit machines.
>
> That's not entirely accurate. The arguments are also mangled on x86-64,
> it's just that in a few cases that goes unnoticed. That's why I
> suggested you try and take your test case (which I assume had been
> passing with flying colours on x86-64) and rearrange the specifiers,
> arguments and expected output string so that the (morally) 32 bit
> arguments end up beyond those-that-end-up-in-the-reg_save_area.
>
> IOWs, it is the 32 bit arguments that are mangled (because they get
> passed as-if they were actually 64 bits), and that applies on all
> architectures; nothing to do with sizeof(long).
Mh, yes, I get your point and I agree that my description does not
really fit what you reported.
I tried what you suggested though, with the current bpf-next/master on x86_64:
BPF_SNPRINTF(out, sizeof(out),
"%u %d %u %d %u %d %u %d %u %d %u %d",
1, -2, 3, -4, 5, -6, 7, -8, 9, -10, 11, -12);
And out is "1 -2 3 -4 5 -6 7 -8 9 -10 11 -12" so i can't seem to be
able to produce the bug you described.
Do you think I'm missing something? Would you try it differently ?
On 23/04/2021 15.26, Florent Revest wrote:
> On Fri, Apr 23, 2021 at 10:50 AM Rasmus Villemoes
> <[email protected]> wrote:
>>
>>> This solves a bug reported by Rasmus Villemoes that would mangle
>>> arguments on 32 bit machines.
>>
>> That's not entirely accurate. The arguments are also mangled on x86-64,
>> it's just that in a few cases that goes unnoticed. That's why I
>> suggested you try and take your test case (which I assume had been
>> passing with flying colours on x86-64) and rearrange the specifiers,
>> arguments and expected output string so that the (morally) 32 bit
>> arguments end up beyond those-that-end-up-in-the-reg_save_area.
>>
>> IOWs, it is the 32 bit arguments that are mangled (because they get
>> passed as-if they were actually 64 bits), and that applies on all
>> architectures; nothing to do with sizeof(long).
>
> Mh, yes, I get your point and I agree that my description does not
> really fit what you reported.
>
> I tried what you suggested though, with the current bpf-next/master on x86_64:
> BPF_SNPRINTF(out, sizeof(out),
> "%u %d %u %d %u %d %u %d %u %d %u %d",
> 1, -2, 3, -4, 5, -6, 7, -8, 9, -10, 11, -12);
>
> And out is "1 -2 3 -4 5 -6 7 -8 9 -10 11 -12" so i can't seem to be
> able to produce the bug you described.
> Do you think I'm missing something? Would you try it differently ?
>
Nah, sorry, I must have misremembered the x86-64 ABI. Re-reading it, it
clearly says as the very first thing "The size of each argument gets
rounded up to eightbytes". So each of the ints that get passed on the
stack do indeed occupy 8 bytes (i.e., the overflow_area pointer gets
adjusted by 8 bytes, for both va_arg(ap, int) and va_arg(ap, long)). So
it will indeed work on x86-64. And probably other 64 bit ABIs behave the
same way (it would make sense) - at least ppc64 and arm64 seem to behave
like that.
So in a round-about way it's probably true that the bug would only be
seen on 32 bit machines, but only because all (relevant) 64 bit arches
seem to, on the ABI level, effectively do argument promotion to u64 anyway.
Rasmus