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

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

[Randy Dunlap]

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".)

Fixes: https://syzkaller.appspot.com/bug?id=0627d4551fdc39bf1ef5d82cd9eef587047f7718

Reported-by: syzbot+6bd77b88c1977c03f584@syzkaller.appspotmail.com
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: stable@vger.kernel.org # many kernel versions
Cc: reiserfs-devel@vger.kernel.org
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Jeff Mahoney <jeffm@suse.com>
Cc: Jan Kara <jack@suse.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: Andrew Morton <akpm@linuxfoundation.org>
---
 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;
 			}
 		}

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

[Andrew Morton]

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()?

--- 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;
_

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

[Jeff Mahoney]

On 4/4/18 9:45 PM, 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()?

Yep.  That's exactly it.

Acked-by: Jeff Mahoney <jeffm@suse.com>

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

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

[Randy Dunlap]

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 <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()?

OK, thanks.

> Yep.  That's exactly it.
> 
> Acked-by: Jeff Mahoney <jeffm@suse.com>
> 
> 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

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@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

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

[Rasmus Villemoes]

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

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

[Jeff Mahoney]

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 <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:

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

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

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]

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 <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()?
> 
> Yep.  That's exactly it.
> 
> Acked-by: Jeff Mahoney <jeffm@suse.com>

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 <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-reiserfs-Fix-warning-for-non-existing-journal-device.patch --]
[-- Type: text/x-patch, Size: 1441 bytes --]

>From 121724c8bb9d5c07ee12718520f6f99b0da0a275 Mon Sep 17 00:00:00 2001
From: Andrew Morton <akpm@linux-foundation.org>
Date: Mon, 9 Apr 2018 16:17:44 +0200
Subject: [PATCH] reiserfs: Fix warning for non-existing journal devices

When a journal device specified as part of mount options does not exist,
reiserfs issues a warking like:

	reiserfs_warning(super,
			 "journal_init_dev: Cannot open '%s': %i",
			 jdev_name, result);

Now this misses a parameter 'id' of reiserfs_warning() which comes
second. As such, the format string is interpreted as an ID and jdev_name as
a format string resulting in funny issues.

Fix the problem by adding missing 'id' argument.

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Acked-by: Jeff Mahoney <jeffm@suse.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/reiserfs/journal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index 70057359fbaf..23148c3ed675 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -2643,7 +2643,7 @@ static int journal_init_dev(struct super_block *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;
-- 
2.13.6

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

[Dmitry Vyukov]

On Mon, Apr 9, 2018 at 4:25 PM, Jan Kara <jack@suse.cz> 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 <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()?
>>
>> Yep.  That's exactly it.
>>
>> Acked-by: Jeff Mahoney <jeffm@suse.com>
>
> 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: syzbot+6bd77b88c1977c03f584@syzkaller.appspotmail.com

as the original reporter.

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

[Jan Kara]

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 <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()?
> > 
> > Yep.  That's exactly it.
> > 
> > Acked-by: Jeff Mahoney <jeffm@suse.com>
> 
> 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 <jack@suse.com>
SUSE Labs, CR