Re: [PATCH?] reiserfs: prevent panic: don't allow %-char in journal dev. name
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 > > > wrote: > > > > > >> From: Randy Dunlap > > >> > > >> 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 > > 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 SUSE Labs, CR
Re: [PATCH?] reiserfs: prevent panic: don't allow %-char in journal dev. name
On Mon, Apr 9, 2018 at 4:25 PM, 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 >> > wrote: >> > >> >> From: Randy Dunlap >> >> >> >> 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 > > 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+6bd77b88c1977c03f...@syzkaller.appspotmail.com as the original reporter.
Re: [PATCH?] reiserfs: prevent panic: don't allow %-char in journal dev. name
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 > > wrote: > > > >> From: Randy Dunlap > >> > >> 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 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 SUSE Labs, CR >From 121724c8bb9d5c07ee12718520f6f99b0da0a275 Mon Sep 17 00:00:00 2001 From: Andrew Morton 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 Signed-off-by: Andrew Morton Acked-by: Jeff Mahoney Signed-off-by: Jan Kara --- 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
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 wrote: >> >>> From: Randy Dunlap >>> >>> 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 > #include > > 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
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
On 2018-04-05 03:45, Andrew Morton wrote: > On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap wrote: > >> From: Randy Dunlap >> >> 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 #include 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
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 wrote: >> >>> From: Randy Dunlap >>> >>> 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 > > 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
On 4/4/18 9:45 PM, Andrew Morton wrote: > On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap wrote: > >> From: Randy Dunlap >> >> 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 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
On Wed, 4 Apr 2018 18:25:16 -0700 Randy Dunlap wrote: > From: Randy Dunlap > > 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; _