* [PATCH] bfs: add sanity check at bfs_fill_super(). @ 2018-05-09 10:35 Tetsuo Handa 2018-05-09 23:06 ` Andrew Morton 2018-05-10 0:53 ` Matthew Wilcox 0 siblings, 2 replies; 21+ messages in thread From: Tetsuo Handa @ 2018-05-09 10:35 UTC (permalink / raw) To: akpm; +Cc: linux-fsdevel, Tetsuo Handa, Tigran Aivazian, syzbot syzbot is reporting too large memory allocation at bfs_fill_super() [1]. Since file system image is corrupted such that bfs_sb->s_start == 0, bfs_fill_super() is trying to allocate 8MB of continuous memory. Fix this by adding a sanity check on bfs_sb->s_start, __GFP_NOWARN and printf(). [1] https://syzkaller.appspot.com/bug?id=16a87c236b951351374a84c8a32f40edbc034e96 Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Reported-by: syzbot <syzbot+71c6b5d68e91149fc8a4@syzkaller.appspotmail.com> Cc: Tigran Aivazian <aivazian.tigran@gmail.com> --- fs/bfs/inode.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c index 9a69392..d81c148 100644 --- a/fs/bfs/inode.c +++ b/fs/bfs/inode.c @@ -350,7 +350,8 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent) s->s_magic = BFS_MAGIC; - if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end)) { + if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) || + le32_to_cpu(bfs_sb->s_start) < BFS_BSIZE) { printf("Superblock is corrupted\n"); goto out1; } @@ -359,9 +360,11 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent) sizeof(struct bfs_inode) + BFS_ROOT_INO - 1; imap_len = (info->si_lasti / 8) + 1; - info->si_imap = kzalloc(imap_len, GFP_KERNEL); - if (!info->si_imap) + info->si_imap = kzalloc(imap_len, GFP_KERNEL | __GFP_NOWARN); + if (!info->si_imap) { + printf("Cannot allocate %u bytes\n", imap_len); goto out1; + } for (i = 0; i < BFS_ROOT_INO; i++) set_bit(i, info->si_imap); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-05-09 10:35 [PATCH] bfs: add sanity check at bfs_fill_super() Tetsuo Handa @ 2018-05-09 23:06 ` Andrew Morton [not found] ` <201805092346.w49NkINl045657@www262.sakura.ne.jp> 2018-05-10 0:53 ` Matthew Wilcox 1 sibling, 1 reply; 21+ messages in thread From: Andrew Morton @ 2018-05-09 23:06 UTC (permalink / raw) To: Tetsuo Handa; +Cc: linux-fsdevel, Tigran Aivazian, syzbot On Wed, 9 May 2018 19:35:04 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote: > syzbot is reporting too large memory allocation at bfs_fill_super() [1]. > Since file system image is corrupted such that bfs_sb->s_start == 0, > bfs_fill_super() is trying to allocate 8MB of continuous memory. Fix this > by adding a sanity check on bfs_sb->s_start, __GFP_NOWARN and printf(). > > [1] https://syzkaller.appspot.com/bug?id=16a87c236b951351374a84c8a32f40edbc034e96 > > ... > > --- a/fs/bfs/inode.c > +++ b/fs/bfs/inode.c > @@ -350,7 +350,8 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent) > > s->s_magic = BFS_MAGIC; > > - if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end)) { > + if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) || > + le32_to_cpu(bfs_sb->s_start) < BFS_BSIZE) { hm, how did you figure out that s_start cannot be less than BFS_BSIZE? >From this, I guess? info->si_lasti = (le32_to_cpu(bfs_sb->s_start) - BFS_BSIZE) / I wonder if this is the most appropriate way to check. > printf("Superblock is corrupted\n"); > goto out1; > } > @@ -359,9 +360,11 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent) > sizeof(struct bfs_inode) > + BFS_ROOT_INO - 1; > imap_len = (info->si_lasti / 8) + 1; > - info->si_imap = kzalloc(imap_len, GFP_KERNEL); > - if (!info->si_imap) > + info->si_imap = kzalloc(imap_len, GFP_KERNEL | __GFP_NOWARN); > + if (!info->si_imap) { > + printf("Cannot allocate %u bytes\n", imap_len); > goto out1; > + } This seems unnecessary - if the kzalloc() fails we'll get the page-allocation-fauilure warning and a nice backtrace, etc. Why suppress all of that and add our custom warning instead? ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <201805092346.w49NkINl045657@www262.sakura.ne.jp>]
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). [not found] ` <201805092346.w49NkINl045657@www262.sakura.ne.jp> @ 2018-05-09 23:53 ` Andrew Morton 2018-06-13 13:33 ` Tetsuo Handa 0 siblings, 1 reply; 21+ messages in thread From: Andrew Morton @ 2018-05-09 23:53 UTC (permalink / raw) To: Tetsuo Handa; +Cc: linux-fsdevel, Tigran Aivazian, syzbot On Thu, 10 May 2018 08:46:18 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > page-allocation-fauilure warning and a nice backtrace, etc. Why > > suppress all of that and add our custom warning instead? > > the intent of this patch is to avoid panic() by panic_on_warn == 1 > due to hitting > > struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags) > { > unsigned int index; > > if (unlikely(size > KMALLOC_MAX_SIZE)) { > WARN_ON_ONCE(!(flags & __GFP_NOWARN)); /* <= this line */ > return NULL; > } > > when size to allocate is controlled by the filesystem image. Well, the same could happen with many many memory-allocation sites. What's special about BFS? If someone sets panic_on_warn=1 then presumably this panic is the behaviour they wanted in this case. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-05-09 23:53 ` Andrew Morton @ 2018-06-13 13:33 ` Tetsuo Handa 2018-06-13 13:49 ` Tigran Aivazian 0 siblings, 1 reply; 21+ messages in thread From: Tetsuo Handa @ 2018-06-13 13:33 UTC (permalink / raw) To: Tigran Aivazian; +Cc: Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On 2018/05/10 8:53, Andrew Morton wrote: > On Thu, 10 May 2018 08:46:18 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > >>> page-allocation-fauilure warning and a nice backtrace, etc. Why >>> suppress all of that and add our custom warning instead? >> >> the intent of this patch is to avoid panic() by panic_on_warn == 1 >> due to hitting >> >> struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags) >> { >> unsigned int index; >> >> if (unlikely(size > KMALLOC_MAX_SIZE)) { >> WARN_ON_ONCE(!(flags & __GFP_NOWARN)); /* <= this line */ >> return NULL; >> } >> >> when size to allocate is controlled by the filesystem image. > > Well, the same could happen with many many memory-allocation sites. > What's special about BFS? If someone sets panic_on_warn=1 then > presumably this panic is the behaviour they wanted in this case. > Tigran, this patch is stalling. Do we want to apply this? Or, ignore as invalid? errors=panic mount option for ext4 case was ignored as invalid. http://lkml.kernel.org/r/CACT4Y+Z+2YW_VALJzzQr6hLsviA=dXk3iFqwVf+P5zqojeC9Zg@mail.gmail.com But I prefer avoiding crashes if we can fix it. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-13 13:33 ` Tetsuo Handa @ 2018-06-13 13:49 ` Tigran Aivazian 2018-06-13 16:00 ` Dmitry Vyukov 2018-06-13 22:09 ` Tetsuo Handa 0 siblings, 2 replies; 21+ messages in thread From: Tigran Aivazian @ 2018-06-13 13:49 UTC (permalink / raw) To: Tetsuo Handa; +Cc: Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs Having read the discussion carefully, I personally prefer to ignore the fix as invalid, because mounting a filesystem image is a privileged operation and if attempting to mount a corrupted image causes a panic, that is no big deal, imho. On 13 June 2018 at 14:33, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2018/05/10 8:53, Andrew Morton wrote: >> On Thu, 10 May 2018 08:46:18 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: >> >>>> page-allocation-fauilure warning and a nice backtrace, etc. Why >>>> suppress all of that and add our custom warning instead? >>> >>> the intent of this patch is to avoid panic() by panic_on_warn == 1 >>> due to hitting >>> >>> struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags) >>> { >>> unsigned int index; >>> >>> if (unlikely(size > KMALLOC_MAX_SIZE)) { >>> WARN_ON_ONCE(!(flags & __GFP_NOWARN)); /* <= this line */ >>> return NULL; >>> } >>> >>> when size to allocate is controlled by the filesystem image. >> >> Well, the same could happen with many many memory-allocation sites. >> What's special about BFS? If someone sets panic_on_warn=1 then >> presumably this panic is the behaviour they wanted in this case. >> > > Tigran, this patch is stalling. Do we want to apply this? Or, ignore as invalid? > > errors=panic mount option for ext4 case was ignored as invalid. > http://lkml.kernel.org/r/CACT4Y+Z+2YW_VALJzzQr6hLsviA=dXk3iFqwVf+P5zqojeC9Zg@mail.gmail.com > > But I prefer avoiding crashes if we can fix it. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-13 13:49 ` Tigran Aivazian @ 2018-06-13 16:00 ` Dmitry Vyukov 2018-06-14 12:23 ` Tigran Aivazian 2018-06-13 22:09 ` Tetsuo Handa 1 sibling, 1 reply; 21+ messages in thread From: Dmitry Vyukov @ 2018-06-13 16:00 UTC (permalink / raw) To: Tigran Aivazian Cc: Tetsuo Handa, Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On Wed, Jun 13, 2018 at 3:49 PM, Tigran Aivazian <aivazian.tigran@gmail.com> wrote: > Having read the discussion carefully, I personally prefer to ignore > the fix as invalid, because mounting a filesystem image is a > privileged operation and if attempting to mount a corrupted image > causes a panic, that is no big deal, imho. Even if not a big deal, but still a bug? Also note that this is kind a chicken and egg problem. The only reason why these operations are privileged is that we have bugs and we are not fixing them. Also keep this in mind whenever you insert anything into usb and automount if turned on ;) You are basically giving permission to that thing to do everything with the machine. Or when you mount any image not created by you with trusted tools that you build yourself from trusted sources with a trusted compiler. Also keep in mind Android and other systems where root is not a trusted entity. > On 13 June 2018 at 14:33, Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> On 2018/05/10 8:53, Andrew Morton wrote: >>> On Thu, 10 May 2018 08:46:18 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: >>> >>>>> page-allocation-fauilure warning and a nice backtrace, etc. Why >>>>> suppress all of that and add our custom warning instead? >>>> >>>> the intent of this patch is to avoid panic() by panic_on_warn == 1 >>>> due to hitting >>>> >>>> struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags) >>>> { >>>> unsigned int index; >>>> >>>> if (unlikely(size > KMALLOC_MAX_SIZE)) { >>>> WARN_ON_ONCE(!(flags & __GFP_NOWARN)); /* <= this line */ >>>> return NULL; >>>> } >>>> >>>> when size to allocate is controlled by the filesystem image. >>> >>> Well, the same could happen with many many memory-allocation sites. >>> What's special about BFS? If someone sets panic_on_warn=1 then >>> presumably this panic is the behaviour they wanted in this case. >>> >> >> Tigran, this patch is stalling. Do we want to apply this? Or, ignore as invalid? >> >> errors=panic mount option for ext4 case was ignored as invalid. >> http://lkml.kernel.org/r/CACT4Y+Z+2YW_VALJzzQr6hLsviA=dXk3iFqwVf+P5zqojeC9Zg@mail.gmail.com >> >> But I prefer avoiding crashes if we can fix it. > > -- > You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/CAK%2B_RLko_OCepN4FCmsaQPAKkt9JNGe8pNRK7SO-onhw5zCneA%40mail.gmail.com. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-13 16:00 ` Dmitry Vyukov @ 2018-06-14 12:23 ` Tigran Aivazian 2018-06-14 12:38 ` Dmitry Vyukov 0 siblings, 1 reply; 21+ messages in thread From: Tigran Aivazian @ 2018-06-14 12:23 UTC (permalink / raw) To: Dmitry Vyukov Cc: Tetsuo Handa, Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs Hello Dmitry, Historically, Unix mount(2) (and mount(8)) are normally privileged unless the filesystem is "trusted" by sysadmin who recorded this fact in the corresponding entry in /etc/fstab (see fstab(5)). I agree that the filesystem mounting code should perform enough validation on the superblock to avoid crashing on an invalid filesystem image. But I disagree that some specific filesystem (and least of all BFS!) should modify the panic_on_warn semantics and replace it with its own private warning message on kernel memory allocation failures. Kind regards, Tigran On 13 June 2018 at 17:00, Dmitry Vyukov <dvyukov@google.com> wrote: > On Wed, Jun 13, 2018 at 3:49 PM, Tigran Aivazian > <aivazian.tigran@gmail.com> wrote: >> Having read the discussion carefully, I personally prefer to ignore >> the fix as invalid, because mounting a filesystem image is a >> privileged operation and if attempting to mount a corrupted image >> causes a panic, that is no big deal, imho. > > Even if not a big deal, but still a bug? > > Also note that this is kind a chicken and egg problem. The only reason > why these operations are privileged is that we have bugs and we are > not fixing them. > > Also keep this in mind whenever you insert anything into usb and > automount if turned on ;) You are basically giving permission to that > thing to do everything with the machine. Or when you mount any image > not created by you with trusted tools that you build yourself from > trusted sources with a trusted compiler. > > Also keep in mind Android and other systems where root is not a trusted entity. > > > >> On 13 June 2018 at 14:33, Tetsuo Handa >> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>> On 2018/05/10 8:53, Andrew Morton wrote: >>>> On Thu, 10 May 2018 08:46:18 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: >>>> >>>>>> page-allocation-fauilure warning and a nice backtrace, etc. Why >>>>>> suppress all of that and add our custom warning instead? >>>>> >>>>> the intent of this patch is to avoid panic() by panic_on_warn == 1 >>>>> due to hitting >>>>> >>>>> struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags) >>>>> { >>>>> unsigned int index; >>>>> >>>>> if (unlikely(size > KMALLOC_MAX_SIZE)) { >>>>> WARN_ON_ONCE(!(flags & __GFP_NOWARN)); /* <= this line */ >>>>> return NULL; >>>>> } >>>>> >>>>> when size to allocate is controlled by the filesystem image. >>>> >>>> Well, the same could happen with many many memory-allocation sites. >>>> What's special about BFS? If someone sets panic_on_warn=1 then >>>> presumably this panic is the behaviour they wanted in this case. >>>> >>> >>> Tigran, this patch is stalling. Do we want to apply this? Or, ignore as invalid? >>> >>> errors=panic mount option for ext4 case was ignored as invalid. >>> http://lkml.kernel.org/r/CACT4Y+Z+2YW_VALJzzQr6hLsviA=dXk3iFqwVf+P5zqojeC9Zg@mail.gmail.com >>> >>> But I prefer avoiding crashes if we can fix it. >> >> -- >> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. >> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. >> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/CAK%2B_RLko_OCepN4FCmsaQPAKkt9JNGe8pNRK7SO-onhw5zCneA%40mail.gmail.com. >> For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-14 12:23 ` Tigran Aivazian @ 2018-06-14 12:38 ` Dmitry Vyukov 2018-06-14 13:05 ` Tigran Aivazian 0 siblings, 1 reply; 21+ messages in thread From: Dmitry Vyukov @ 2018-06-14 12:38 UTC (permalink / raw) To: Tigran Aivazian Cc: Tetsuo Handa, Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On Thu, Jun 14, 2018 at 2:23 PM, Tigran Aivazian <aivazian.tigran@gmail.com> wrote: > Hello Dmitry, > > Historically, Unix mount(2) (and mount(8)) are normally privileged > unless the filesystem is "trusted" by sysadmin who recorded this fact > in the corresponding entry in /etc/fstab (see fstab(5)). I agree that > the filesystem mounting code should perform enough validation on the > superblock to avoid crashing on an invalid filesystem image. But I > disagree that some specific filesystem (and least of all BFS!) should > modify the panic_on_warn semantics and replace it with its own private > warning message on kernel memory allocation failures. This is not a general memory allocation failure. Nothing is printed on memory allocation failures. This is a very specific case of memory allocation failure, namely when kernel code asks for too large block, such large that it can't be possible allocated regardless of amount of available memory. For allocations with user-controllable size kernel code either needs to use __GFP_NOWARN, or (better) impose own limits such that it can always be allocated with kmalloc. Consider, currently we can have a bfs image that works fine on one kernel, but fails to mount on another just because it happens so that one could allocate 4MB with kmalloc, but another can't (different allocator/different settings/different kernel revision). This looks like pretty unfortunate property for persistent data formats in general. > Kind regards, > Tigran > > On 13 June 2018 at 17:00, Dmitry Vyukov <dvyukov@google.com> wrote: >> On Wed, Jun 13, 2018 at 3:49 PM, Tigran Aivazian >> <aivazian.tigran@gmail.com> wrote: >>> Having read the discussion carefully, I personally prefer to ignore >>> the fix as invalid, because mounting a filesystem image is a >>> privileged operation and if attempting to mount a corrupted image >>> causes a panic, that is no big deal, imho. >> >> Even if not a big deal, but still a bug? >> >> Also note that this is kind a chicken and egg problem. The only reason >> why these operations are privileged is that we have bugs and we are >> not fixing them. >> >> Also keep this in mind whenever you insert anything into usb and >> automount if turned on ;) You are basically giving permission to that >> thing to do everything with the machine. Or when you mount any image >> not created by you with trusted tools that you build yourself from >> trusted sources with a trusted compiler. >> >> Also keep in mind Android and other systems where root is not a trusted entity. >> >> >> >>> On 13 June 2018 at 14:33, Tetsuo Handa >>> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>>> On 2018/05/10 8:53, Andrew Morton wrote: >>>>> On Thu, 10 May 2018 08:46:18 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: >>>>> >>>>>>> page-allocation-fauilure warning and a nice backtrace, etc. Why >>>>>>> suppress all of that and add our custom warning instead? >>>>>> >>>>>> the intent of this patch is to avoid panic() by panic_on_warn == 1 >>>>>> due to hitting >>>>>> >>>>>> struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags) >>>>>> { >>>>>> unsigned int index; >>>>>> >>>>>> if (unlikely(size > KMALLOC_MAX_SIZE)) { >>>>>> WARN_ON_ONCE(!(flags & __GFP_NOWARN)); /* <= this line */ >>>>>> return NULL; >>>>>> } >>>>>> >>>>>> when size to allocate is controlled by the filesystem image. >>>>> >>>>> Well, the same could happen with many many memory-allocation sites. >>>>> What's special about BFS? If someone sets panic_on_warn=1 then >>>>> presumably this panic is the behaviour they wanted in this case. >>>>> >>>> >>>> Tigran, this patch is stalling. Do we want to apply this? Or, ignore as invalid? >>>> >>>> errors=panic mount option for ext4 case was ignored as invalid. >>>> http://lkml.kernel.org/r/CACT4Y+Z+2YW_VALJzzQr6hLsviA=dXk3iFqwVf+P5zqojeC9Zg@mail.gmail.com >>>> >>>> But I prefer avoiding crashes if we can fix it. >>> >>> -- >>> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group. >>> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com. >>> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/CAK%2B_RLko_OCepN4FCmsaQPAKkt9JNGe8pNRK7SO-onhw5zCneA%40mail.gmail.com. >>> For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-14 12:38 ` Dmitry Vyukov @ 2018-06-14 13:05 ` Tigran Aivazian 2018-06-14 13:12 ` Dmitry Vyukov 2018-06-14 13:28 ` Tetsuo Handa 0 siblings, 2 replies; 21+ messages in thread From: Tigran Aivazian @ 2018-06-14 13:05 UTC (permalink / raw) To: Dmitry Vyukov Cc: Tetsuo Handa, Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On 14 June 2018 at 13:38, Dmitry Vyukov <dvyukov@google.com> wrote: > Consider, currently we can have a bfs image that works fine on one > kernel, but fails to mount on another just because it happens so that > one could allocate 4MB with kmalloc, but another can't (different > allocator/different settings/different kernel revision). Yes, but this would only happen _without_ the validation proposed by Tetsuo Handa. If we check s_start then the invalid enormous allocation request will not be made and what you describe won't not happen. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-14 13:05 ` Tigran Aivazian @ 2018-06-14 13:12 ` Dmitry Vyukov 2018-06-14 13:28 ` Tetsuo Handa 1 sibling, 0 replies; 21+ messages in thread From: Dmitry Vyukov @ 2018-06-14 13:12 UTC (permalink / raw) To: Tigran Aivazian Cc: Tetsuo Handa, Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On Thu, Jun 14, 2018 at 3:05 PM, Tigran Aivazian <aivazian.tigran@gmail.com> wrote: > On 14 June 2018 at 13:38, Dmitry Vyukov <dvyukov@google.com> wrote: >> Consider, currently we can have a bfs image that works fine on one >> kernel, but fails to mount on another just because it happens so that >> one could allocate 4MB with kmalloc, but another can't (different >> allocator/different settings/different kernel revision). > > Yes, but this would only happen _without_ the validation proposed by > Tetsuo Handa. If we check s_start then the invalid enormous allocation > request will not be made and what you describe won't not happen. Agree. If we do a sanity check first, then __GFP_NOWARN is actually useful: it will detect the cases where a filesystem can be mounted on one machine but not on another (and will also double check that our sanity is really sane). ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-14 13:05 ` Tigran Aivazian 2018-06-14 13:12 ` Dmitry Vyukov @ 2018-06-14 13:28 ` Tetsuo Handa 2018-06-14 15:13 ` Tigran Aivazian 1 sibling, 1 reply; 21+ messages in thread From: Tetsuo Handa @ 2018-06-14 13:28 UTC (permalink / raw) To: Tigran Aivazian, Dmitry Vyukov Cc: Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On 2018/06/14 22:05, Tigran Aivazian wrote: > On 14 June 2018 at 13:38, Dmitry Vyukov <dvyukov@google.com> wrote: >> Consider, currently we can have a bfs image that works fine on one >> kernel, but fails to mount on another just because it happens so that >> one could allocate 4MB with kmalloc, but another can't (different >> allocator/different settings/different kernel revision). > > Yes, but this would only happen _without_ the validation proposed by > Tetsuo Handa. If we check s_start then the invalid enormous allocation > request will not be made and what you describe won't not happen. > What is possible largest value for imap_len ? info->si_lasti = (le32_to_cpu(bfs_sb->s_start) - BFS_BSIZE) / sizeof(struct bfs_inode) + BFS_ROOT_INO - 1; imap_len = (info->si_lasti / 8) + 1; info->si_imap = kzalloc(imap_len, GFP_KERNEL); Since sizeof(struct bfs_inode) is 64 and bfs_sb->s_start is unsigned 32bits integer (where constraints is BFS_BSIZE <= bfs_sb->s_start <= bfs_sb->s_end), theoretically it is possible to assign bfs_sb->s_start > 2GB (apart from whether such value makes sense). Then, isn't it possible that imap_len > 4M and still hit KMALLOC_MAX_SIZE limit? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-14 13:28 ` Tetsuo Handa @ 2018-06-14 15:13 ` Tigran Aivazian 2018-06-14 16:15 ` Tigran Aivazian 0 siblings, 1 reply; 21+ messages in thread From: Tigran Aivazian @ 2018-06-14 15:13 UTC (permalink / raw) To: Tetsuo Handa Cc: Dmitry Vyukov, Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On 14 June 2018 at 14:28, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > What is possible largest value for imap_len ? > > info->si_lasti = (le32_to_cpu(bfs_sb->s_start) - BFS_BSIZE) / sizeof(struct bfs_inode) + BFS_ROOT_INO - 1; > imap_len = (info->si_lasti / 8) + 1; > info->si_imap = kzalloc(imap_len, GFP_KERNEL); > > Since sizeof(struct bfs_inode) is 64 and bfs_sb->s_start is unsigned 32bits integer > (where constraints is BFS_BSIZE <= bfs_sb->s_start <= bfs_sb->s_end), theoretically > it is possible to assign bfs_sb->s_start > 2GB (apart from whether such value makes > sense). Then, isn't it possible that imap_len > 4M and still hit KMALLOC_MAX_SIZE limit? You are correct, but the proper fix should be to restrict imap_len to whatever the maximum value allowed by BFS filesystem layout and reject anything beyond it. I will try to remember what it was from the notes I made when I wrote BFS back in 1999. Please wait (possibly a few days) and I will let you know what those values are. Kind regards, Tigran ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-14 15:13 ` Tigran Aivazian @ 2018-06-14 16:15 ` Tigran Aivazian 2018-06-14 19:00 ` Tigran Aivazian 0 siblings, 1 reply; 21+ messages in thread From: Tigran Aivazian @ 2018-06-14 16:15 UTC (permalink / raw) To: Tetsuo Handa Cc: Dmitry Vyukov, Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On 14 June 2018 at 16:13, Tigran Aivazian <aivazian.tigran@gmail.com> wrote: > On 14 June 2018 at 14:28, Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> What is possible largest value for imap_len ? >> >> info->si_lasti = (le32_to_cpu(bfs_sb->s_start) - BFS_BSIZE) / sizeof(struct bfs_inode) + BFS_ROOT_INO - 1; >> imap_len = (info->si_lasti / 8) + 1; >> info->si_imap = kzalloc(imap_len, GFP_KERNEL); >> >> Since sizeof(struct bfs_inode) is 64 and bfs_sb->s_start is unsigned 32bits integer >> (where constraints is BFS_BSIZE <= bfs_sb->s_start <= bfs_sb->s_end), theoretically >> it is possible to assign bfs_sb->s_start > 2GB (apart from whether such value makes >> sense). Then, isn't it possible that imap_len > 4M and still hit KMALLOC_MAX_SIZE limit? > > You are correct, but the proper fix should be to restrict imap_len to > whatever the maximum value allowed by BFS filesystem layout and reject > anything beyond it. I will try to remember what it was from the notes > I made when I wrote BFS back in 1999. Please wait (possibly a few > days) and I will let you know what those values are. Actually, a more accurate sanity check for the value of s_start should be (patch against 4.16.3): --- fs/bfs/inode.c.0 2018-06-14 16:50:52.136792126 +0100 +++ fs/bfs/inode.c 2018-06-14 16:51:49.344792119 +0100 @@ -350,7 +350,8 @@ s->s_magic = BFS_MAGIC; - if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end)) { + if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) || + le32_to_cpu(bfs_sb->s_start) < sizeof(struct bfs_super_block) + sizeof(struct bfs_dirent)) { printf("Superblock is corrupted\n"); goto out1; } However, that doesn't address the issue of the _upper_ limit of s_start, i.e. it can still get (on an invalid image pretending to be BFS) arbitrarily large and cause the allocation to fail as you described. I will dig a bit more (in my memories :) and try to come up with the check which doesn't reject a valid BFS image and at the same time restricts s_start (or imap_len which ultimately depends on it) sufficiently to prevent wild kernel memory allocation requests. Btw, I included in the WikiPedia article "Boot File System" a reference to the original "BFS kernel support" webpage from those ancient days: http://ftp.linux.org.uk/pub/linux/iBCS/bfs/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-14 16:15 ` Tigran Aivazian @ 2018-06-14 19:00 ` Tigran Aivazian 2018-06-14 22:18 ` Tetsuo Handa 0 siblings, 1 reply; 21+ messages in thread From: Tigran Aivazian @ 2018-06-14 19:00 UTC (permalink / raw) To: Tetsuo Handa Cc: Dmitry Vyukov, Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On 14 June 2018 at 17:15, Tigran Aivazian <aivazian.tigran@gmail.com> wrote: > On 14 June 2018 at 16:13, Tigran Aivazian <aivazian.tigran@gmail.com> wrote: >> On 14 June 2018 at 14:28, Tetsuo Handa >> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>> What is possible largest value for imap_len ? >>> >>> info->si_lasti = (le32_to_cpu(bfs_sb->s_start) - BFS_BSIZE) / sizeof(struct bfs_inode) + BFS_ROOT_INO - 1; >>> imap_len = (info->si_lasti / 8) + 1; >>> info->si_imap = kzalloc(imap_len, GFP_KERNEL); >>> >>> Since sizeof(struct bfs_inode) is 64 and bfs_sb->s_start is unsigned 32bits integer >>> (where constraints is BFS_BSIZE <= bfs_sb->s_start <= bfs_sb->s_end), theoretically >>> it is possible to assign bfs_sb->s_start > 2GB (apart from whether such value makes >>> sense). Then, isn't it possible that imap_len > 4M and still hit KMALLOC_MAX_SIZE limit? >> >> You are correct, but the proper fix should be to restrict imap_len to >> whatever the maximum value allowed by BFS filesystem layout and reject >> anything beyond it. I will try to remember what it was from the notes >> I made when I wrote BFS back in 1999. Please wait (possibly a few >> days) and I will let you know what those values are. > > Actually, a more accurate sanity check for the value of s_start should > be (patch against 4.16.3): > > --- fs/bfs/inode.c.0 2018-06-14 16:50:52.136792126 +0100 > +++ fs/bfs/inode.c 2018-06-14 16:51:49.344792119 +0100 > @@ -350,7 +350,8 @@ > > s->s_magic = BFS_MAGIC; > > - if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end)) { > + if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) || > + le32_to_cpu(bfs_sb->s_start) < sizeof(struct bfs_super_block) > + sizeof(struct bfs_dirent)) { > printf("Superblock is corrupted\n"); > goto out1; > } > > However, that doesn't address the issue of the _upper_ limit of > s_start, i.e. it can still get (on an invalid image pretending to be > BFS) arbitrarily large and cause the allocation to fail as you > described. I will dig a bit more (in my memories :) and try to come up > with the check which doesn't reject a valid BFS image and at the same > time restricts s_start (or imap_len which ultimately depends on it) > sufficiently to prevent wild kernel memory allocation requests. > > Btw, I included in the WikiPedia article "Boot File System" a > reference to the original "BFS kernel support" webpage from those > ancient days: http://ftp.linux.org.uk/pub/linux/iBCS/bfs/ Ah, it turned out easier than I thought! The maximum number of inodes of a BFS filesystem is 512, so an inode map cannot be longer than 65 bytes. Well, we can be generous and restrict imap_len to 128 and be done with it :) Namely, if the calculated imap_len turns out to be greater than 128, then something is definitely wrong and the filesystem image should be rejected as corrupted. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-14 19:00 ` Tigran Aivazian @ 2018-06-14 22:18 ` Tetsuo Handa 2018-06-15 10:45 ` Tigran Aivazian 0 siblings, 1 reply; 21+ messages in thread From: Tetsuo Handa @ 2018-06-14 22:18 UTC (permalink / raw) To: Tigran Aivazian Cc: Dmitry Vyukov, Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On 2018/06/15 4:00, Tigran Aivazian wrote: >> However, that doesn't address the issue of the _upper_ limit of >> s_start, i.e. it can still get (on an invalid image pretending to be >> BFS) arbitrarily large and cause the allocation to fail as you >> described. I will dig a bit more (in my memories :) and try to come up >> with the check which doesn't reject a valid BFS image and at the same >> time restricts s_start (or imap_len which ultimately depends on it) >> sufficiently to prevent wild kernel memory allocation requests. >> >> Btw, I included in the WikiPedia article "Boot File System" a >> reference to the original "BFS kernel support" webpage from those >> ancient days: http://ftp.linux.org.uk/pub/linux/iBCS/bfs/ > > Ah, it turned out easier than I thought! The maximum number of inodes > of a BFS filesystem is 512, so an inode map cannot be longer than 65 > bytes. Well, we can be generous and restrict imap_len to 128 and be > done with it :) > > Namely, if the calculated imap_len turns out to be greater than 128, > then something is definitely wrong and the filesystem image should be > rejected as corrupted. > So, the constraint is if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) || le32_to_cpu(bfs_sb->s_end) > What_is_the_number_here) you can write the fix yourself... ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-14 22:18 ` Tetsuo Handa @ 2018-06-15 10:45 ` Tigran Aivazian 0 siblings, 0 replies; 21+ messages in thread From: Tigran Aivazian @ 2018-06-15 10:45 UTC (permalink / raw) To: Tetsuo Handa Cc: Dmitry Vyukov, Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On 14 June 2018 at 23:18, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2018/06/15 4:00, Tigran Aivazian wrote: >> Ah, it turned out easier than I thought! The maximum number of inodes >> of a BFS filesystem is 512, so an inode map cannot be longer than 65 >> bytes. Well, we can be generous and restrict imap_len to 128 and be >> done with it :) >> >> Namely, if the calculated imap_len turns out to be greater than 128, >> then something is definitely wrong and the filesystem image should be >> rejected as corrupted. >> > So, the constraint is > > if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) || > le32_to_cpu(bfs_sb->s_end) > What_is_the_number_here) > > you can write the fix yourself... No, s_end has nothing to do with the number of inodes, it is to do with the actual data blocks. Yes, I am writing the fix myself and will test it under 4.17.1 to which I switched my Ubuntu desktop just now. Thanks, Tigran ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-13 13:49 ` Tigran Aivazian 2018-06-13 16:00 ` Dmitry Vyukov @ 2018-06-13 22:09 ` Tetsuo Handa 2018-06-14 7:38 ` Tigran Aivazian 1 sibling, 1 reply; 21+ messages in thread From: Tetsuo Handa @ 2018-06-13 22:09 UTC (permalink / raw) To: Tigran Aivazian; +Cc: Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On 2018/06/13 22:49, Tigran Aivazian wrote: > Having read the discussion carefully, I personally prefer to ignore > the fix as invalid, because mounting a filesystem image is a > privileged operation and if attempting to mount a corrupted image > causes a panic, that is no big deal, imho. While this report is triggered by a crafted filesystem image, I don't think that a legitimate but huge filesystem image crashes the system by hitting (size > KMALLOC_MAX_SIZE) path is nice. While filesystem should try to avoid such large allocation, there is no need to crash the system just because kmalloc() failed. e.g. http://lkml.kernel.org/r/927f24d4-b0c3-8192-5723-c314f38b4292@iogearbox.net ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-13 22:09 ` Tetsuo Handa @ 2018-06-14 7:38 ` Tigran Aivazian 2018-06-14 10:45 ` Tetsuo Handa 0 siblings, 1 reply; 21+ messages in thread From: Tigran Aivazian @ 2018-06-14 7:38 UTC (permalink / raw) To: Tetsuo Handa; +Cc: Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On 13 June 2018 at 23:09, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > While this report is triggered by a crafted filesystem image, I don't think that > a legitimate but huge filesystem image crashes the system by hitting > (size > KMALLOC_MAX_SIZE) path is nice. While filesystem should try to avoid > such large allocation, there is no need to crash the system just because > kmalloc() failed. > > e.g. http://lkml.kernel.org/r/927f24d4-b0c3-8192-5723-c314f38b4292@iogearbox.net Ok, could you please show me the very final version of the suggested patch, please? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-14 7:38 ` Tigran Aivazian @ 2018-06-14 10:45 ` Tetsuo Handa 2018-06-14 12:11 ` Tigran Aivazian 0 siblings, 1 reply; 21+ messages in thread From: Tetsuo Handa @ 2018-06-14 10:45 UTC (permalink / raw) To: Tigran Aivazian; +Cc: Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs On 2018/06/14 16:38, Tigran Aivazian wrote: > On 13 June 2018 at 23:09, Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> While this report is triggered by a crafted filesystem image, I don't think that >> a legitimate but huge filesystem image crashes the system by hitting >> (size > KMALLOC_MAX_SIZE) path is nice. While filesystem should try to avoid >> such large allocation, there is no need to crash the system just because >> kmalloc() failed. >> >> e.g. http://lkml.kernel.org/r/927f24d4-b0c3-8192-5723-c314f38b4292@iogearbox.net > > Ok, could you please show me the very final version of the suggested > patch, please? > (Which patch did you mean? Hmm, I paste both patches instead of waiting for your answer.) commit 0c54bbdb89992eeff50866b64366a1a0cbd0e6fb Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Sat May 26 14:15:41 2018 +1000 bfs: add sanity check at bfs_fill_super() syzbot is reporting too large memory allocation at bfs_fill_super() [1]. Since file system image is corrupted such that bfs_sb->s_start == 0, bfs_fill_super() is trying to allocate 8MB of continuous memory. Fix this by adding a sanity check on bfs_sb->s_start, __GFP_NOWARN and printf(). [1] https://syzkaller.appspot.com/bug?id=16a87c236b951351374a84c8a32f40edbc034e96 Link: http://lkml.kernel.org/r/1525862104-3407-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Reported-by: syzbot <syzbot+71c6b5d68e91149fc8a4@syzkaller.appspotmail.com> Reviewed-by: Andrew Morton <akpm@linux-foundation.org> Cc: Tigran Aivazian <aivazian.tigran@gmail.com> Cc: Matthew Wilcox <willy@infradead.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au> diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c index 9a69392..d81c148 100644 --- a/fs/bfs/inode.c +++ b/fs/bfs/inode.c @@ -350,7 +350,8 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent) s->s_magic = BFS_MAGIC; - if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end)) { + if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) || + le32_to_cpu(bfs_sb->s_start) < BFS_BSIZE) { printf("Superblock is corrupted\n"); goto out1; } @@ -359,9 +360,11 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent) sizeof(struct bfs_inode) + BFS_ROOT_INO - 1; imap_len = (info->si_lasti / 8) + 1; - info->si_imap = kzalloc(imap_len, GFP_KERNEL); - if (!info->si_imap) + info->si_imap = kzalloc(imap_len, GFP_KERNEL | __GFP_NOWARN); + if (!info->si_imap) { + printf("Cannot allocate %u bytes\n", imap_len); goto out1; + } for (i = 0; i < BFS_ROOT_INO; i++) set_bit(i, info->si_imap); commit a343993c518ce252b62ec00ac06bccfb1d17129d Author: Bj旦rn T旦pel <bjorn.topel@intel.com> Date: Mon Jun 11 13:57:12 2018 +0200 xsk: silence warning on memory allocation failure syzkaller reported a warning from xdp_umem_pin_pages(): WARNING: CPU: 1 PID: 4537 at mm/slab_common.c:996 kmalloc_slab+0x56/0x70 mm/slab_common.c:996 ... __do_kmalloc mm/slab.c:3713 [inline] __kmalloc+0x25/0x760 mm/slab.c:3727 kmalloc_array include/linux/slab.h:634 [inline] kcalloc include/linux/slab.h:645 [inline] xdp_umem_pin_pages net/xdp/xdp_umem.c:205 [inline] xdp_umem_reg net/xdp/xdp_umem.c:318 [inline] xdp_umem_create+0x5c9/0x10f0 net/xdp/xdp_umem.c:349 xsk_setsockopt+0x443/0x550 net/xdp/xsk.c:531 __sys_setsockopt+0x1bd/0x390 net/socket.c:1935 __do_sys_setsockopt net/socket.c:1946 [inline] __se_sys_setsockopt net/socket.c:1943 [inline] __x64_sys_setsockopt+0xbe/0x150 net/socket.c:1943 do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287 entry_SYSCALL_64_after_hwframe+0x49/0xbe This is a warning about attempting to allocate more than KMALLOC_MAX_SIZE memory. The request originates from userspace, and if the request is too big, the kernel is free to deny its allocation. In this patch, the failed allocation attempt is silenced with __GFP_NOWARN. Fixes: c0c77d8fb787 ("xsk: add user memory registration support sockopt") Reported-by: syzbot+4abadc5d69117b346506@syzkaller.appspotmail.com Signed-off-by: Bj旦rn T旦pel <bjorn.topel@intel.com> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c index b9ef487..f47abb4 100644 --- a/net/xdp/xdp_umem.c +++ b/net/xdp/xdp_umem.c @@ -204,7 +204,8 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem) long npgs; int err; - umem->pgs = kcalloc(umem->npgs, sizeof(*umem->pgs), GFP_KERNEL); + umem->pgs = kcalloc(umem->npgs, sizeof(*umem->pgs), + GFP_KERNEL | __GFP_NOWARN); if (!umem->pgs) return -ENOMEM; ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-06-14 10:45 ` Tetsuo Handa @ 2018-06-14 12:11 ` Tigran Aivazian 0 siblings, 0 replies; 21+ messages in thread From: Tigran Aivazian @ 2018-06-14 12:11 UTC (permalink / raw) To: Tetsuo Handa; +Cc: Andrew Morton, linux-fsdevel, syzbot, syzkaller-bugs Thank you. Please see comments below: On 14 June 2018 at 11:45, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > (Which patch did you mean? Hmm, I paste both patches instead of waiting for your answer.) I only meant the BFS patch. > commit 0c54bbdb89992eeff50866b64366a1a0cbd0e6fb > Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Date: Sat May 26 14:15:41 2018 +1000 > > bfs: add sanity check at bfs_fill_super() > > syzbot is reporting too large memory allocation at bfs_fill_super() [1]. > Since file system image is corrupted such that bfs_sb->s_start == 0, > bfs_fill_super() is trying to allocate 8MB of continuous memory. Fix this > by adding a sanity check on bfs_sb->s_start, __GFP_NOWARN and printf(). > > [1] https://syzkaller.appspot.com/bug?id=16a87c236b951351374a84c8a32f40edbc034e96 > > Link: http://lkml.kernel.org/r/1525862104-3407-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Reported-by: syzbot <syzbot+71c6b5d68e91149fc8a4@syzkaller.appspotmail.com> > Reviewed-by: Andrew Morton <akpm@linux-foundation.org> > Cc: Tigran Aivazian <aivazian.tigran@gmail.com> > Cc: Matthew Wilcox <willy@infradead.org> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au> > > diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c > index 9a69392..d81c148 100644 > --- a/fs/bfs/inode.c > +++ b/fs/bfs/inode.c > @@ -350,7 +350,8 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent) > > s->s_magic = BFS_MAGIC; > > - if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end)) { > + if (le32_to_cpu(bfs_sb->s_start) > le32_to_cpu(bfs_sb->s_end) || > + le32_to_cpu(bfs_sb->s_start) < BFS_BSIZE) { > printf("Superblock is corrupted\n"); > goto out1; > } Yes, this is acceptable. > @@ -359,9 +360,11 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent) > sizeof(struct bfs_inode) > + BFS_ROOT_INO - 1; > imap_len = (info->si_lasti / 8) + 1; > - info->si_imap = kzalloc(imap_len, GFP_KERNEL); > - if (!info->si_imap) > + info->si_imap = kzalloc(imap_len, GFP_KERNEL | __GFP_NOWARN); > + if (!info->si_imap) { > + printf("Cannot allocate %u bytes\n", imap_len); > goto out1; > + } This is unnecessary. As Andrew Morton pointed out, if people set panic_on_warn then they do really want a panic on allocation failure warnings. Please update the patch with just the above sanity check for s_start < BFS_BSIZE. Kind regards, Tigran ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] bfs: add sanity check at bfs_fill_super(). 2018-05-09 10:35 [PATCH] bfs: add sanity check at bfs_fill_super() Tetsuo Handa 2018-05-09 23:06 ` Andrew Morton @ 2018-05-10 0:53 ` Matthew Wilcox 1 sibling, 0 replies; 21+ messages in thread From: Matthew Wilcox @ 2018-05-10 0:53 UTC (permalink / raw) To: Tetsuo Handa; +Cc: akpm, linux-fsdevel, Tigran Aivazian, syzbot On Wed, May 09, 2018 at 07:35:04PM +0900, Tetsuo Handa wrote: > syzbot is reporting too large memory allocation at bfs_fill_super() [1]. > Since file system image is corrupted such that bfs_sb->s_start == 0, > bfs_fill_super() is trying to allocate 8MB of continuous memory. Fix this > by adding a sanity check on bfs_sb->s_start, __GFP_NOWARN and printf(). > > [1] https://syzkaller.appspot.com/bug?id=16a87c236b951351374a84c8a32f40edbc034e96 Looking at that C reproducer, it looks like it's trying to fuzz filesystem images: https://syzkaller.appspot.com/text?tag=ReproC&x=15b834a7800000 Do we really want this? I was under the impression that we simply do not care about attackers who have control of the media, and having syzbot generate thousands of bugs that nobody will work on is not a good use of anybody's time. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2018-06-15 10:45 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-09 10:35 [PATCH] bfs: add sanity check at bfs_fill_super() Tetsuo Handa 2018-05-09 23:06 ` Andrew Morton [not found] ` <201805092346.w49NkINl045657@www262.sakura.ne.jp> 2018-05-09 23:53 ` Andrew Morton 2018-06-13 13:33 ` Tetsuo Handa 2018-06-13 13:49 ` Tigran Aivazian 2018-06-13 16:00 ` Dmitry Vyukov 2018-06-14 12:23 ` Tigran Aivazian 2018-06-14 12:38 ` Dmitry Vyukov 2018-06-14 13:05 ` Tigran Aivazian 2018-06-14 13:12 ` Dmitry Vyukov 2018-06-14 13:28 ` Tetsuo Handa 2018-06-14 15:13 ` Tigran Aivazian 2018-06-14 16:15 ` Tigran Aivazian 2018-06-14 19:00 ` Tigran Aivazian 2018-06-14 22:18 ` Tetsuo Handa 2018-06-15 10:45 ` Tigran Aivazian 2018-06-13 22:09 ` Tetsuo Handa 2018-06-14 7:38 ` Tigran Aivazian 2018-06-14 10:45 ` Tetsuo Handa 2018-06-14 12:11 ` Tigran Aivazian 2018-05-10 0:53 ` Matthew Wilcox
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).