All of lore.kernel.org
 help / color / mirror / Atom feed
* [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

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

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

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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.