| Message ID | 1417825533-13081-2-git-send-email-benjamin@southpole.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
But that is not needed anymore ? regards Antti On 12/06/2014 02:25 AM, Benjamin Larsson wrote: > Using this driver with the attach dvb model might trigger a use > after free when unloading the driver. With this change the driver > will always fail on unload instead of randomly crash depending > on if the memory has been reused or not. > > Signed-off-by: Benjamin Larsson <benjamin@southpole.se> > --- > drivers/staging/media/mn88472/mn88472.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/staging/media/mn88472/mn88472.c b/drivers/staging/media/mn88472/mn88472.c > index 36ef39b..a9d5f0a 100644 > --- a/drivers/staging/media/mn88472/mn88472.c > +++ b/drivers/staging/media/mn88472/mn88472.c > @@ -489,6 +489,7 @@ static int mn88472_remove(struct i2c_client *client) > > regmap_exit(dev->regmap[0]); > > + memset(dev, 0, sizeof(*dev)); > kfree(dev); > > return 0; >
On 12/06/2014 05:29 PM, Antti Palosaari wrote: > But that is not needed anymore ? > > regards > Antti Chances are that more devices with the mn8847x chips appear. Someone somewhere might try to use this demod with the old dvb attach model during development. Adding this memset will make the unload issue appear all the time instead of randomly. But this patch doesn't really matter so feel free to NACK it. I just wanted to post it for completeness. I do think it is good practice to set pointers to null generally as that would have saved me several days of work of whentracking down this bug. The current dvb framework contain several other cases where pointers are feed'd but not nulled. MvH Benjamin Larsson -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/2014 08:08 PM, Benjamin Larsson wrote: > On 12/06/2014 05:29 PM, Antti Palosaari wrote: >> But that is not needed anymore ? >> >> regards >> Antti > > Chances are that more devices with the mn8847x chips appear. Someone > somewhere might try to use this demod with the old dvb attach model > during development. Adding this memset will make the unload issue appear > all the time instead of randomly. But this patch doesn't really matter > so feel free to NACK it. I just wanted to post it for completeness. > > I do think it is good practice to set pointers to null generally as that > would have saved me several days of work of whentracking down this bug. > The current dvb framework contain several other cases where pointers are > feed'd but not nulled. There is kzfree() for that, but still I am very unsure should we start zeroing memory upon release driver has allocated, or just relase it using kfree. regards Antti
On 12/06/2014 07:37 PM, Antti Palosaari wrote: >> >> I do think it is good practice to set pointers to null generally as that >> would have saved me several days of work of whentracking down this bug. >> The current dvb framework contain several other cases where pointers are >> feed'd but not nulled. > > There is kzfree() for that, but still I am very unsure should we start > zeroing memory upon release driver has allocated, or just relase it > using kfree. > > regards > Antti Well I guess I am biased as I have spent lots of time finding a bug that probably wouldn't exist if the policy was that drivers always should set their memory to zero before it is free'd. Maybe we should have a compile time override so that all free calls zeroes the memory before the actual free? Maybe there already is this kind of feature? MvH Benjamin Larsson -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/2014 09:26 PM, Benjamin Larsson wrote: > On 12/06/2014 07:37 PM, Antti Palosaari wrote: >>> >>> I do think it is good practice to set pointers to null generally as that >>> would have saved me several days of work of whentracking down this bug. >>> The current dvb framework contain several other cases where pointers are >>> feed'd but not nulled. >> >> There is kzfree() for that, but still I am very unsure should we start >> zeroing memory upon release driver has allocated, or just relase it >> using kfree. >> >> regards >> Antti > > Well I guess I am biased as I have spent lots of time finding a bug that > probably wouldn't exist if the policy was that drivers always should set > their memory to zero before it is free'd. Just because you zero memory before it is freed doesn't mean it stays zeroed. As soon as it is freed some other process might take that memory and fill it up again. So zeroing is pointless and in fact will only *hide* bugs. The only reason I know of for zeroing memory before freeing is if that memory contains sensitive information and you want to make sure it is gone from memory. You can turn on the kmemcheck kernel option when compiling the kernel to test for accesses to uninitialized memory if you suspect you have a bug in that area. Anyway: Nacked-by: Hans Verkuil <hans.verkuil@cisco.com> Regards, Hans > Maybe we should have a compile > time override so that all free calls zeroes the memory before the actual > free? Maybe there already is this kind of feature? > > MvH > Benjamin Larsson > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/19/2015 02:15 PM, Hans Verkuil wrote: > On 12/06/2014 09:26 PM, Benjamin Larsson wrote: >> >> Well I guess I am biased as I have spent lots of time finding a bug that >> probably wouldn't exist if the policy was that drivers always should set >> their memory to zero before it is free'd. > > Just because you zero memory before it is freed doesn't mean it stays zeroed. > As soon as it is freed some other process might take that memory and fill it > up again. So zeroing is pointless and in fact will only *hide* bugs. > Well in this specific case NOT zeroing the memory it actually hid a use after free bug. So stating that it is pointless and that it will only hide bugs is not correct at least for this case. MvH Benjamin Larsson -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/media/mn88472/mn88472.c b/drivers/staging/media/mn88472/mn88472.c index 36ef39b..a9d5f0a 100644 --- a/drivers/staging/media/mn88472/mn88472.c +++ b/drivers/staging/media/mn88472/mn88472.c @@ -489,6 +489,7 @@ static int mn88472_remove(struct i2c_client *client) regmap_exit(dev->regmap[0]); + memset(dev, 0, sizeof(*dev)); kfree(dev); return 0;
Using this driver with the attach dvb model might trigger a use after free when unloading the driver. With this change the driver will always fail on unload instead of randomly crash depending on if the memory has been reused or not. Signed-off-by: Benjamin Larsson <benjamin@southpole.se> --- drivers/staging/media/mn88472/mn88472.c | 1 + 1 file changed, 1 insertion(+)