Re: [PATCH] scsi: sd: Jump to out_free_index if device_add{,_disk}() fail

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

On luned? 28 marzo 2022 16:38:53 CEST Dan Carpenter wrote:
> On Mon, Mar 28, 2022 at 10:44:52AM +0200, Fabio M. De Francesco wrote:
> > Currently, if device_add() or device_add_disk() fail, the code jumps to
> > the "out" label. Doing so we get a memory leak as Syzbot reports.[1]
> > 
> > Fix this bug by jumping to the "out_free_index" label.
> > 
> > [1] https://groups.google.com/g/syzkaller-bugs/c/BvuqG6YGb6I
> > 
> > Reported-and-tested-by: syzbot+f08c77040fa163a75a46@syzkaller.appspotmail.com
> > Fixes: 2a7a891f4c40 ("scsi: sd: Add error handling support for add_disk()")
> > Fixes: 265dfe8ebbab ("scsi: sd: Free scsi_disk device via put_device()")
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  drivers/scsi/sd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index a390679cf458..61fcf653ef5a 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -3434,7 +3434,7 @@ static int sd_probe(struct device *dev)
> >  	error = device_add(&sdkp->disk_dev);
> >  	if (error) {
> >  		put_device(&sdkp->disk_dev);
> > -		goto out;
> > +		goto out_free_index;
> 
> The put_device calls scsi_disk_release() so this change will introduce
> use after frees and double frees.

Yes, correct. I've just looked at how put_device() is implemented. I didn't 
know how it works until you made me notice. Thanks!

Aside this I sent another diff to Syzbot. Today, at 4.30 CET, it replied again
that, after applying and testing my new patch, Syzkaller was not anymore able 
to trigger the memory leak that it had reported.

This is the new diff...

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a390679cf458..6fac62f00b37 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3433,6 +3433,7 @@ static int sd_probe(struct device *dev)
 
        error = device_add(&sdkp->disk_dev);
        if (error) {
+               ida_free(&sd_index_ida, index);
                put_device(&sdkp->disk_dev);
                goto out;
        }
@@ -3474,6 +3475,7 @@ static int sd_probe(struct device *dev)
 
        error = device_add_disk(dev, gd, NULL);
        if (error) {
+               ida_free(&sd_index_ida, index);
                put_device(&sdkp->disk_dev);
                goto out;
        }

As it can be seen, I tried to simply free the IDA and jump to the "out"
label (exactly as it is in the current code).

The test passed today at 04.30 CET but, can it really be that not freeing 
the allocation of the IDA could trigger that memory leak? I'm not so sure, 
therefore, I'll wait for comments before submitting any v2.

> There is a larger process issue here.  We need to figure out why syzbot
> did not detect that this patch introduces bugs.

This is something that the people who run Syzbot/Syzkaller should help to
figure out.

Regards,

Fabio M. De Francesco

> regards,
> dan carpenter