Re: [PATCH?] reiserfs: prevent panic: don't allow %-char in journal dev. name

[Rasmus Villemoes]

On 2018-04-05 03:45, Andrew Morton wrote:
> On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap <rdunlap@xxxxxxxxxxxxx> wrote:
> 
>> From: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
>>
>> 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