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: [email protected]
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 <[email protected]>
---
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;
}
dev_set_drvdata(dev, sdkp);
@@ -3475,7 +3475,7 @@ static int sd_probe(struct device *dev)
error = device_add_disk(dev, gd, NULL);
if (error) {
put_device(&sdkp->disk_dev);
- goto out;
+ goto out_free_index;
}
if (sdkp->capacity)
--
2.34.1
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: [email protected]
> 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 <[email protected]>
> ---
> 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.
There is a larger process issue here. We need to figure out why syzbot
did not detect that this patch introduces bugs.
regards,
dan carpenter
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: [email protected]
> > 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 <[email protected]>
> > ---
> > 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
On marted? 29 marzo 2022 09:57:53 CEST Dan Carpenter wrote:
> On Tue, Mar 29, 2022 at 10:47:45AM +0300, Dan Carpenter wrote:
> > No, this patch is wrong. That is supposed to be freed in scsi_disk_release()
> > but apparently that's not getting called. Is the ref counting off?
> >
>
> Yeah. The device_add() needs a matching device_del().
>
> regards,
> dan carpenter
>
Thanks, Dan.
I've just just checked other similar code and saw the following pattern
whenever we have an error:
device_del(dev);
put_device(dev);
Therefore, I suppose that you are suggesting the following simple change:
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a390679cf458..13d96d0f9dde 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3474,6 +3474,7 @@ static int sd_probe(struct device *dev)
error = device_add_disk(dev, gd, NULL);
if (error) {
+ device_del(&sdkp->disk_dev);
put_device(&sdkp->disk_dev);
goto out;
}
Did I get it?
Regards,
Fabio
No, this patch is wrong. That is supposed to be freed in scsi_disk_release()
but apparently that's not getting called. Is the ref counting off?
On Tue, Mar 29, 2022 at 08:18:16AM +0200, Fabio M. De Francesco wrote:
> > 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.
Yeah. Right now syzbot just says "TEST PASSED" but it doesn't give a
complete dmesg. There were other leaks on this path. Were they not
detected? We can't know without looking at the dmesg.
regards,
dan carpenter
On Tue, Mar 29, 2022 at 10:28:24AM +0200, Fabio M. De Francesco wrote:
> Therefore, I suppose that you are suggesting the following simple change:
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index a390679cf458..13d96d0f9dde 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3474,6 +3474,7 @@ static int sd_probe(struct device *dev)
>
> error = device_add_disk(dev, gd, NULL);
> if (error) {
> + device_del(&sdkp->disk_dev);
> put_device(&sdkp->disk_dev);
> goto out;
> }
>
> Did I get it?
Yep.
regards,
dan carpenter
On Tue, Mar 29, 2022 at 10:47:45AM +0300, Dan Carpenter wrote:
> No, this patch is wrong. That is supposed to be freed in scsi_disk_release()
> but apparently that's not getting called. Is the ref counting off?
>
Yeah. The device_add() needs a matching device_del().
regards,
dan carpenter
On Tue, Mar 29, 2022 at 08:18:16AM +0200, Fabio M. De Francesco wrote:
> 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...
Yes, this new diff makes much more sense.