Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [syzbot] WARNING in internal_create_group
To:     Christoph Hellwig <hch@lst.de>, Hillf Danton <hdanton@sina.com>
Cc:     syzbot <syzbot+9937dc42271cd87d4b98@syzkaller.appspotmail.com>,
        linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com,
        Eric Sandeen <sandeen@redhat.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
References: <000000000000bd7c8a05c719ecf2@google.com>
 <20210721033703.949-1-hdanton@sina.com> <20210721043034.GB7444@lst.de>
From:   Pavel Skripkin <paskripkin@gmail.com>
Message-ID: <39ac87a8-42ac-acf7-11eb-ba0b6a9f4a95@gmail.com>
Date:   Thu, 12 Aug 2021 00:37:08 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <20210721043034.GB7444@lst.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/21/21 7:30 AM, Christoph Hellwig wrote:
> On Wed, Jul 21, 2021 at 11:37:03AM +0800, Hillf Danton wrote:
>> On Tue, 20 Jul 2021 11:53:27 -0700
>> >syzbot has found a reproducer for the following issue on:
>> >
>> >HEAD commit:    8cae8cd89f05 seq_file: disallow extremely large seq buffer..
>> >git tree:       upstream
>> >console output: https://syzkaller.appspot.com/x/log.txt?x=116f92ec300000
>> >kernel config:  https://syzkaller.appspot.com/x/.config?x=7273c75708b55890
>> >dashboard link: https://syzkaller.appspot.com/bug?extid=9937dc42271cd87d4b98
>> >syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=15fc287c300000
>> >C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=178cbf6a300000
> 
> <snip>
> 
>> >WARNING: CPU: 0 PID: 8435 at fs/sysfs/group.c:116 internal_create_group+0x911/0xb20 fs/sysfs/group.c:116
> 
> <snip>
> 
>> The device_add(ddev) in register_disk() may fail but it proceeds to register
>> block queue even at the failure ... this falls in the class of known issue
>> given the comment line.
>> 
>>  * FIXME: error handling
>>  */
>> static void __device_add_disk(struct device *parent, struct gendisk *disk,
> 
> Yes, Luis is working on actually fixing this - but it requires changes
> to every single block driver.  How does a cap on the seq_buf size
> propagate here, though?
> 

Hi!

I've looked into this, and, I think, we can add sanity check for 
first_minor. If user will pass too big index (syzbot's repro passes 
1048576) this value will be shifted to part_shift and then truncated to 
byte in __device_add_disk() and assigned to dev->devt. User may be 
confused about why he passed 1048576, but sysfs warns about duplicate 
creation of /dev/block/43:0

So, these type of errors can be handled before passing wrong values to 
sysfs API like this:

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index c38317979f74..600e9bab5d43 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1725,7 +1725,17 @@ static int nbd_dev_add(int index)
  	refcount_set(&nbd->refs, 1);
  	INIT_LIST_HEAD(&nbd->list);
  	disk->major = NBD_MAJOR;
+
+	/* Too big first_minor can cause duplicate creation of
+	 * sysfs files/links, since first_minor will be truncated to
+	 * byte in __device_add_disk().
+	 */
  	disk->first_minor = index << part_shift;
+	if (disk->first_minor > 0xff) {
+		err = -EINVAL;
+		goto out_free_idr;
+	}
+
  	disk->minors = 1 << part_shift;
  	disk->fops = &nbd_fops;
  	disk->private_data = nbd;


What to do you think about it?


With regards,
Pavel Skripkin