From: Randy Dunlap <[email protected]>
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".)
Fixes: https://syzkaller.appspot.com/bug?id=0627d4551fdc39bf1ef5d82cd9eef587047f7718
Reported-by: [email protected]
Signed-off-by: Randy Dunlap <[email protected]>
Cc: [email protected] # many kernel versions
Cc: [email protected]
Cc: Alexander Viro <[email protected]>
Cc: Jeff Mahoney <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Artem Bityutskiy <[email protected]>
Cc: Andrew Morton <[email protected]>
---
fs/reiserfs/super.c | 11 +++++++++++
1 file changed, 11 insertions(+)
--- lnx-416.orig/fs/reiserfs/super.c
+++ lnx-416/fs/reiserfs/super.c
@@ -1239,6 +1239,8 @@ static int reiserfs_parse_options(struct
}
if (c == 'j') {
+ char *badfmt; // jdev_name (arg) cannot contain '%'
+
if (arg && *arg && jdev_name) {
/* Hm, already assigned? */
if (*jdev_name) {
@@ -1248,6 +1250,15 @@ static int reiserfs_parse_options(struct
"be %s", *jdev_name);
return 0;
}
+
+ badfmt = strchr(arg, '%');
+ if (badfmt) {
+ printk(KERN_WARNING "reiserfs: "
+ "journal device name "
+ "is invalid: %s",
+ arg);
+ return 0;
+ }
*jdev_name = arg;
}
}
On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap <[email protected]> wrote:
> From: Randy Dunlap <[email protected]>
>
> 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()?
--- a/fs/reiserfs/journal.c~a
+++ a/fs/reiserfs/journal.c
@@ -2643,7 +2643,7 @@ static int journal_init_dev(struct super
if (IS_ERR(journal->j_dev_bd)) {
result = PTR_ERR(journal->j_dev_bd);
journal->j_dev_bd = NULL;
- reiserfs_warning(super,
+ reiserfs_warning(super, "sh-457",
"journal_init_dev: Cannot open '%s': %i",
jdev_name, result);
return result;
_
On 4/4/18 9:45 PM, Andrew Morton wrote:
> On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap <[email protected]> wrote:
>
>> From: Randy Dunlap <[email protected]>
>>
>> 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()?
Yep. That's exactly it.
Acked-by: Jeff Mahoney <[email protected]>
Thanks,
-Jeff
> --- a/fs/reiserfs/journal.c~a
> +++ a/fs/reiserfs/journal.c
> @@ -2643,7 +2643,7 @@ static int journal_init_dev(struct super
> if (IS_ERR(journal->j_dev_bd)) {
> result = PTR_ERR(journal->j_dev_bd);
> journal->j_dev_bd = NULL;
> - reiserfs_warning(super,
> + reiserfs_warning(super, "sh-457",
> "journal_init_dev: Cannot open '%s': %i",
> jdev_name, result);
> return result;
> _
>
>
--
Jeff Mahoney
SUSE Labs
On 04/04/2018 06:48 PM, Jeff Mahoney wrote:
> On 4/4/18 9:45 PM, Andrew Morton wrote:
>> On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap <[email protected]> wrote:
>>
>>> From: Randy Dunlap <[email protected]>
>>>
>>> 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()?
OK, thanks.
> Yep. That's exactly it.
>
> Acked-by: Jeff Mahoney <[email protected]>
>
> Thanks,
>
> -Jeff
>
>> --- a/fs/reiserfs/journal.c~a
>> +++ a/fs/reiserfs/journal.c
>> @@ -2643,7 +2643,7 @@ static int journal_init_dev(struct super
>> if (IS_ERR(journal->j_dev_bd)) {
>> result = PTR_ERR(journal->j_dev_bd);
>> journal->j_dev_bd = NULL;
>> - reiserfs_warning(super,
>> + reiserfs_warning(super, "sh-457",
>> "journal_init_dev: Cannot open '%s': %i",
>> jdev_name, result);
>> return result;
>> _
>>
>>
>
>
--
~Randy
On 2018-04-05 03:45, Andrew Morton wrote:
> On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap <[email protected]> wrote:
>
>> From: Randy Dunlap <[email protected]>
>>
>> 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
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
On 4/5/18 5:04 AM, Rasmus Villemoes wrote:
> On 2018-04-05 03:45, Andrew Morton wrote:
>> On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap <[email protected]> wrote:
>>
>>> From: Randy Dunlap <[email protected]>
>>>
>>> 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:
Yep. There are a bunch of ways that this is broken, but it's been "good
enough" for so long that no fix has landed. Once upon a time, I wanted
to fix this by adding something similar to %pV that allowed the caller
to pass a set of handlers for additional types. That didn't make it off
the ground.
There's another issue where we assume that % will only be followed by a
single character. That won't cause runtime issues, but it will end up
putting those additional characters in the output.
Lastly, again not a runtime issue, is that the spinlock only covers
formatting the buffer. It doesn't cover printing it. You can end up
with part of the error buffer containing the format of another warning.
I'm working up something to fix most of the above. I'll post it later
today or Monday.
-Jeff
> 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
>
--
Jeff Mahoney
SUSE Labs
On Mon, Apr 9, 2018 at 4:25 PM, Jan Kara <[email protected]> wrote:
> On Wed 04-04-18 21:48:53, Jeff Mahoney wrote:
>> On 4/4/18 9:45 PM, Andrew Morton wrote:
>> > On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap <[email protected]> wrote:
>> >
>> >> From: Randy Dunlap <[email protected]>
>> >>
>> >> 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()?
>>
>> Yep. That's exactly it.
>>
>> Acked-by: Jeff Mahoney <[email protected]>
>
> Thanks. I've picked up the patch from Andrew, added his Signed-off-by (OK,
> Andrew?), wrote a proper changelog and pushed it to my tree. The result is
> attached.
Hi Jan,
Please also add:
Reported-by: [email protected]
as the original reporter.
On Mon 09-04-18 16:25:17, Jan Kara wrote:
> On Wed 04-04-18 21:48:53, Jeff Mahoney wrote:
> > On 4/4/18 9:45 PM, Andrew Morton wrote:
> > > On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap <[email protected]> wrote:
> > >
> > >> From: Randy Dunlap <[email protected]>
> > >>
> > >> 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()?
> >
> > Yep. That's exactly it.
> >
> > Acked-by: Jeff Mahoney <[email protected]>
>
> Thanks. I've picked up the patch from Andrew, added his Signed-off-by (OK,
> Andrew?), wrote a proper changelog and pushed it to my tree. The result is
> attached.
Ah, now I've noticed Andrew pushed the patch to his tree. Removing mine and
sorry for the noise.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed 04-04-18 21:48:53, Jeff Mahoney wrote:
> On 4/4/18 9:45 PM, Andrew Morton wrote:
> > On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap <[email protected]> wrote:
> >
> >> From: Randy Dunlap <[email protected]>
> >>
> >> 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()?
>
> Yep. That's exactly it.
>
> Acked-by: Jeff Mahoney <[email protected]>
Thanks. I've picked up the patch from Andrew, added his Signed-off-by (OK,
Andrew?), wrote a proper changelog and pushed it to my tree. The result is
attached.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR