| Message ID | 83c4ab9bbca911aad62343154eabfa1af077b021.1449570042.git.tilman@imap.cc |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Tilman, On di, 2015-12-08 at 12:00 +0100, Tilman Schmidt wrote: > When shutting down the device, the struct ser_cardstate must not be > kfree()d immediately after the call to platform_device_unregister() > since the embedded struct platform_device is still in use. > Move the kfree() call to the release method instead. > > Signed-off-by: Tilman Schmidt <tilman@imap.cc> > Fixes: 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)") > Reported-by: Sasha Levin <sasha.levin@oracle.com> > --- > drivers/isdn/gigaset/ser-gigaset.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/isdn/gigaset/ser-gigaset.c > b/drivers/isdn/gigaset/ser-gigaset.c > index d8771b5..2693cb2 100644 > --- a/drivers/isdn/gigaset/ser-gigaset.c > +++ b/drivers/isdn/gigaset/ser-gigaset.c > @@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate > *cs) > tasklet_kill(&cs->write_tasklet); > if (!cs->hw.ser) > return; > - dev_set_drvdata(&cs->hw.ser->dev.dev, NULL); > platform_device_unregister(&cs->hw.ser->dev); > - kfree(cs->hw.ser); > - cs->hw.ser = NULL; > } > > static void gigaset_device_release(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > + struct cardstate *cs = dev_get_drvdata(dev); > > /* adapted from platform_device_release() in > drivers/base/platform.c */ > kfree(dev->platform_data); > kfree(pdev->resource); > + > + if (!cs) > + return; > + dev_set_drvdata(dev, NULL); dev equals cs->hw.ser->dev.dev, doesn't it? So what does setting cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us? > + kfree(cs->hw.ser); > + cs->hw.ser = NULL; I might be missing something, but what does setting this to NULL buy us here? (I realize that I'm asking questions to code that isn't actually new but only moved around, but I think that's still an opportunity to have another look at that code.) > } > > /* Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 09.12.2015 um 00:12 schrieb Paul Bolle: >> --- a/drivers/isdn/gigaset/ser-gigaset.c >> +++ b/drivers/isdn/gigaset/ser-gigaset.c >> @@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate >> *cs) >> tasklet_kill(&cs->write_tasklet); >> if (!cs->hw.ser) >> return; >> - dev_set_drvdata(&cs->hw.ser->dev.dev, NULL); >> platform_device_unregister(&cs->hw.ser->dev); >> - kfree(cs->hw.ser); >> - cs->hw.ser = NULL; >> } >> >> static void gigaset_device_release(struct device *dev) >> { >> struct platform_device *pdev = to_platform_device(dev); >> + struct cardstate *cs = dev_get_drvdata(dev); >> >> /* adapted from platform_device_release() in >> drivers/base/platform.c */ >> kfree(dev->platform_data); >> kfree(pdev->resource); >> + >> + if (!cs) >> + return; >> + dev_set_drvdata(dev, NULL); > > dev equals cs->hw.ser->dev.dev, doesn't it? Correct. > So what does setting > cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us? We're freeing cs->hw.ser, not cs->hw.ser->dev. Clearing the reference to cs from the device structure before freeing cs guards against possible use-after-free. >> + kfree(cs->hw.ser); >> + cs->hw.ser = NULL; > > I might be missing something, but what does setting this to NULL buy us > here? Just defensive programming. Guarding against possible use-after-free or double-free. > > (I realize that I'm asking questions to code that isn't actually new but > only moved around, but I think that's still an opportunity to have > another look at that code.) I'm a big fan of one change per patch. If we also want to modify the moved code then that should be done in a separate patch. It makes bisecting so much easier. Same reason why I separated out patch 3/3. And btw same reason why I think patch 1/3 should go in as-is, as an obvious fix to commit f34d7a5b, and any concerns about whether those tests are useful should be addressed by a separate patch. Regards, Tilman
On wo, 2015-12-09 at 12:10 +0100, Tilman Schmidt wrote: > Am 09.12.2015 um 00:12 schrieb Paul Bolle: > > So what does setting > > cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy > > us? > > We're freeing cs->hw.ser, not cs->hw.ser->dev. > Clearing the reference to cs from the device structure before freeing > cs guards against possible use-after-free. > > > > + kfree(cs->hw.ser); > > > + cs->hw.ser = NULL; > > > > I might be missing something, but what does setting this to NULL buy > > us here? > > Just defensive programming. Guarding against possible use-after-free > or double-free. I'm inclined to think this is not the best way to guard against such nasty bugs. But then again, I'm only a few months into my shift of looking after the gigaset drivers and haven't had to track down such bugs yet. But I'd be surprised if many other drivers do it that way and think this is a job for (tree wide) debugging tools. But, whatever the merits of our views, we can defer this discussion to some future date. See below. > I'm a big fan of one change per patch. If we also want to modify the > moved code then that should be done in a separate patch. It makes > bisecting so much easier. Same reason why I separated out patch 3/3. Fair enough. Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tilman, On 12/09/2015 03:10 AM, Tilman Schmidt wrote: > Am 09.12.2015 um 00:12 schrieb Paul Bolle: > >>> --- a/drivers/isdn/gigaset/ser-gigaset.c >>> +++ b/drivers/isdn/gigaset/ser-gigaset.c >>> @@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate >>> *cs) >>> tasklet_kill(&cs->write_tasklet); >>> if (!cs->hw.ser) >>> return; >>> - dev_set_drvdata(&cs->hw.ser->dev.dev, NULL); >>> platform_device_unregister(&cs->hw.ser->dev); >>> - kfree(cs->hw.ser); >>> - cs->hw.ser = NULL; >>> } >>> >>> static void gigaset_device_release(struct device *dev) >>> { >>> struct platform_device *pdev = to_platform_device(dev); >>> + struct cardstate *cs = dev_get_drvdata(dev); >>> >>> /* adapted from platform_device_release() in >>> drivers/base/platform.c */ >>> kfree(dev->platform_data); >>> kfree(pdev->resource); >>> + >>> + if (!cs) >>> + return; >>> + dev_set_drvdata(dev, NULL); This is of marginal value and (I think) unnecessary; it implies the core will use the device after release, which would trigger many problems if true. >> dev equals cs->hw.ser->dev.dev, doesn't it? > > Correct. > >> So what does setting >> cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us? > > We're freeing cs->hw.ser, not cs->hw.ser->dev. > Clearing the reference to cs from the device structure before freeing cs > guards against possible use-after-free. > >>> + kfree(cs->hw.ser); >>> + cs->hw.ser = NULL; This pattern is common, and defends against much more common driver bugs. Unfortunately, much of the good this pattern is intended to do in finding use-after-free bugs is undone by explicit tests for null everywhere else. Not saying that's the case here; rather, generally speaking. Like the if (!tty && !tty->ops && ....) code. Better just to let it crash. Regards, Peter Hurley >> I might be missing something, but what does setting this to NULL buy us >> here? > > Just defensive programming. Guarding against possible use-after-free or > double-free. > >> >> (I realize that I'm asking questions to code that isn't actually new but >> only moved around, but I think that's still an opportunity to have >> another look at that code.) > > I'm a big fan of one change per patch. If we also want to modify the > moved code then that should be done in a separate patch. It makes > bisecting so much easier. Same reason why I separated out patch 3/3. And > btw same reason why I think patch 1/3 should go in as-is, as an obvious > fix to commit f34d7a5b, and any concerns about whether those tests are > useful should be addressed by a separate patch. > > Regards, > Tilman > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Peter, Am 10.12.2015 um 15:04 schrieb Peter Hurley: >>>> --- a/drivers/isdn/gigaset/ser-gigaset.c >>>> +++ b/drivers/isdn/gigaset/ser-gigaset.c >>>> @@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate >>>> *cs) >>>> tasklet_kill(&cs->write_tasklet); >>>> if (!cs->hw.ser) >>>> return; >>>> - dev_set_drvdata(&cs->hw.ser->dev.dev, NULL); >>>> platform_device_unregister(&cs->hw.ser->dev); >>>> - kfree(cs->hw.ser); >>>> - cs->hw.ser = NULL; >>>> } >>>> >>>> static void gigaset_device_release(struct device *dev) >>>> { >>>> struct platform_device *pdev = to_platform_device(dev); >>>> + struct cardstate *cs = dev_get_drvdata(dev); >>>> >>>> /* adapted from platform_device_release() in >>>> drivers/base/platform.c */ >>>> kfree(dev->platform_data); >>>> kfree(pdev->resource); >>>> + >>>> + if (!cs) >>>> + return; >>>> + dev_set_drvdata(dev, NULL); > > This is of marginal value and (I think) unnecessary; it implies > the core will use the device after release, which would trigger > many problems if true. Agreed, but I'm just moving existing code here. Dropping the dev_set_drvdata() call would be an unrelated change which should be done in a separate patch if I understand the rules correctly. Regards, Tilman
diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c index d8771b5..2693cb2 100644 --- a/drivers/isdn/gigaset/ser-gigaset.c +++ b/drivers/isdn/gigaset/ser-gigaset.c @@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate *cs) tasklet_kill(&cs->write_tasklet); if (!cs->hw.ser) return; - dev_set_drvdata(&cs->hw.ser->dev.dev, NULL); platform_device_unregister(&cs->hw.ser->dev); - kfree(cs->hw.ser); - cs->hw.ser = NULL; } static void gigaset_device_release(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); + struct cardstate *cs = dev_get_drvdata(dev); /* adapted from platform_device_release() in drivers/base/platform.c */ kfree(dev->platform_data); kfree(pdev->resource); + + if (!cs) + return; + dev_set_drvdata(dev, NULL); + kfree(cs->hw.ser); + cs->hw.ser = NULL; } /*
When shutting down the device, the struct ser_cardstate must not be kfree()d immediately after the call to platform_device_unregister() since the embedded struct platform_device is still in use. Move the kfree() call to the release method instead. Signed-off-by: Tilman Schmidt <tilman@imap.cc> Fixes: 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)") Reported-by: Sasha Levin <sasha.levin@oracle.com> --- drivers/isdn/gigaset/ser-gigaset.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)