Messages in this thread | | | Subject | Re: [PATCH?] reiserfs: prevent panic: don't allow %-char in journal dev. name | From | Rasmus Villemoes <> | Date | Fri, 6 Apr 2018 15:45:14 +0200 |
| |
On 2018-04-05 11:04, Rasmus Villemoes wrote: > On 2018-04-05 03:45, Andrew Morton wrote: >> >> 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...
OK, so maybe -mregparm=3 would be the thing making i386 behave like x86-64 wrt. varargs, but no, when calling a variadic function, gcc pushes all arguments on the stack, and va_list is still just a pointer (passed by value to vsprintf) into the stack.
It is only a problem when the format string contains ordinary specifiers before a reiserfs-specific one, and such calls happen to be rare, but not non-existing. One example would be reiserfs_warning(tb->tb_sb, "vs-12339", "%s (%b)", which, bh);. Ok, treating which as a buffer_head would probably just give some garbage numbers. But
"reiserfs-16100", "STATDATA, index %d, type 0x%x, %h", vi->vi_index, vi->vi_type, vi->vi_ih
ends up treating vi->vi_index as a struct item_head*, no?
Rasmus
| |