lkml.org 
[lkml]   [2018]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH?] reiserfs: prevent panic: don't allow %-char in journal dev. name
From
Date
On 2018-04-05 03:45, Andrew Morton wrote:
> On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap <rdunlap@infradead.org> wrote:
>
>> From: Randy Dunlap <rdunlap@infradead.org>
>>
>> If the reiserfs mount option's journal name contains a '%' character,
>> it can lead to a WARN_ONCE() in lib/vsprintf.c::format_decode(),
>> saying: "Please remove unsupported %/ in format string."
>> That's OK until panic_on_warn is set, at which point it's dead, Jim.
>>
>> To placate this situation, check the journal name string for a '%'
>> character and return an error if one is found. Also print a warning
>> (one that won't panic the kernel) about the invalid journal name (e.g.):
>>
>> reiserfs: journal device name is invalid: %/file0
>>
>> (In this example, the caller app specified the journal device name as
>> "%/file0".)
>>
>
> Well, that is a valid filename and we should support it...
>
> Isn't the bug in journal_init_dev()?

Urgh. At first I was about to reply that the real bug was in reiserfs.h
for failing to annotate __reiserfs_warning with __printf(). But digging
into it, it turns out that it implements its own printf extensions, so
that's obviously a non-starter. Now, one thing is that some of those
extension clash with existing standard modifiers (%z and %h, so if
someone adds a correct %zu thing to print a size_t in reiserfs things
will break). But, and I hope I'm wrong about this and just hasn't had
enough coffee, this seems completely broken:

while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
*k = 0;

p += vsprintf(p, fmt1, args);

switch (what) {
case 'k':
sprintf_le_key(p, va_arg(args, struct
reiserfs_key *));
break;

On architectures where va_list is a typedef for a one-element array of
some struct (x86-64), that works ok, because the vsprintf call can and
does update the args metadata. But when args is just a pointer into the
stack (i386), we don't know how much vsprintf consumed, and end up
consuming the same arguments again - only this time we may interpret
some random integer as a struct pointer...

A minimal program showing the difference:

#include <stdio.h>
#include <stdarg.h>

void f(const char *dummy, ...)
{
va_list ap;
int i;

va_start(ap, dummy);
for (i = 0; i < 5; ++i) {
vprintf("%d\n", ap);
printf("%d\n", va_arg(ap, int));
}
va_end(ap);
}

int main(int argc, char *argv[])
{
f("bla", 1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
return 0;
}

Compiling for native (x86-64), this produces $(seq 10). But with -m32,
one gets 1,1,2,2,3,3,4,4,5,5.

Assuming reiserfs (at least its debugging infrastructure) isn't broken
on a bunch of architectures, I'm obviously missing something
fundamental. Please enlighten me.

Rasmus

\
 
 \ /
  Last update: 2018-04-05 11:05    [W:0.078 / U:0.948 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site