Re: [syzbot] WARNING in internal_create_group

[Pavel Skripkin]

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