From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:36944 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964964AbeFNQPi (ORCPT ); Thu, 14 Jun 2018 12:15:38 -0400 Received: by mail-wm0-f66.google.com with SMTP id r125-v6so13063309wmg.2 for ; Thu, 14 Jun 2018 09:15:37 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1525862104-3407-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp> <20180509160658.c37bef542a8ee5245a13917b@linux-foundation.org> <201805092346.w49NkINl045657@www262.sakura.ne.jp> <20180509165321.3b2b1313fde0f007c1a5a015@linux-foundation.org> <9ef86114-02d6-b243-203d-fbbdab95a6fa@I-love.SAKURA.ne.jp> From: Tigran Aivazian Date: Thu, 14 Jun 2018 17:15:36 +0100 Message-ID: Subject: Re: [PATCH] bfs: add sanity check at bfs_fill_super(). To: Tetsuo Handa Cc: Dmitry Vyukov , Andrew Morton , linux-fsdevel , syzbot , syzkaller-bugs Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 14 June 2018 at 16:13, Tigran Aivazian wrote: > On 14 June 2018 at 14:28, Tetsuo Handa > 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/