bfs: add sanity check at bfs_fill_super().

Commit Message

Tetsuo Handa June 14, 2018, 10:45 a.m. UTC

On 2018/06/14 16:38, Tigran Aivazian wrote:
> On 13 June 2018 at 23:09, Tetsuo Handa
> <[e-mail]> 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/[e-mail]
> 
> 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 <[e-mail]>
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/[e-mail]
    Signed-off-by: Tetsuo Handa <[e-mail]>
    Reported-by: syzbot <[e-mail]>
    Reviewed-by: Andrew Morton <[e-mail]>
    Cc: Tigran Aivazian <[e-mail]>
    Cc: Matthew Wilcox <[e-mail]>
    Signed-off-by: Andrew Morton <[e-mail]>
    Signed-off-by: Stephen Rothwell <[e-mail]>

Comments

Tigran Aivazian June 14, 2018, 12:11 p.m. UTC | #1

Thank you. Please see comments below:

On 14 June 2018 at 11:45, Tetsuo Handa
<[e-mail]> 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 <[e-mail]>
> 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/[e-mail]
>     Signed-off-by: Tetsuo Handa <[e-mail]>
>     Reported-by: syzbot <[e-mail]>
>     Reviewed-by: Andrew Morton <[e-mail]>
>     Cc: Tigran Aivazian <[e-mail]>
>     Cc: Matthew Wilcox <[e-mail]>
>     Signed-off-by: Andrew Morton <[e-mail]>
>     Signed-off-by: Stephen Rothwell <[e-mail]>
>
> 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

Patch

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 <[e-mail]>
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: [e-mail]
    Signed-off-by: Bj旦rn T旦pel <[e-mail]>
    Signed-off-by: Daniel Borkmann <[e-mail]>

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;