Re: [syzbot] WARNING in internal_create_group

[Pavel Skripkin <paskripkin@gmail.com>]

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