Message ID | 20090930152542.3ef753ee@hyperion.delvare (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
At Wed, 30 Sep 2009 15:25:42 +0200, Jean Delvare wrote: > > If i2c device probing fails, then there is no driver to dereference > after calling i2c_new_device(). Stop assuming that probing will always > succeed, to avoid NULL pointer dereferences. We have an easier access > to the driver anyway. > > Reported-by: Tim Shepard <shep@alum.mit.edu> > Signed-off-by: Jean Delvare <khali@linux-fr.org> > Cc: Johannes Berg <johannes@sipsolutions.net> > --- > The code is similar to the one in therm_adt746x, for which Tim reported > a real-world oops, so it should be fixed ASAP. Jean, thanks for the patch. I'm just wondering whether the additional NULL check of client->driver would be enough? If yes, sound/aoa/onyx.c has it, at least, and we can add the similar checks to the rest, too. Takashi > > sound/aoa/codecs/onyx.c | 4 +++- > sound/aoa/codecs/tas.c | 4 +++- > sound/ppc/keywest.c | 4 +++- > 3 files changed, 9 insertions(+), 3 deletions(-) > > --- linux-2.6.32-rc1.orig/sound/aoa/codecs/onyx.c 2009-09-30 15:13:12.000000000 +0200 > +++ linux-2.6.32-rc1/sound/aoa/codecs/onyx.c 2009-09-30 15:13:58.000000000 +0200 > @@ -996,6 +996,8 @@ static void onyx_exit_codec(struct aoa_c > onyx->codec.soundbus_dev->detach_codec(onyx->codec.soundbus_dev, onyx); > } > > +static struct i2c_driver onyx_driver; > + > static int onyx_create(struct i2c_adapter *adapter, > struct device_node *node, > int addr) > @@ -1027,7 +1029,7 @@ static int onyx_create(struct i2c_adapte > * Let i2c-core delete that device on driver removal. > * This is safe because i2c-core holds the core_lock mutex for us. > */ > - list_add_tail(&client->detected, &client->driver->clients); > + list_add_tail(&client->detected, &onyx_driver.clients); > return 0; > } > > --- linux-2.6.32-rc1.orig/sound/aoa/codecs/tas.c 2009-09-30 15:13:12.000000000 +0200 > +++ linux-2.6.32-rc1/sound/aoa/codecs/tas.c 2009-09-30 15:13:58.000000000 +0200 > @@ -882,6 +882,8 @@ static void tas_exit_codec(struct aoa_co > } > > > +static struct i2c_driver tas_driver; > + > static int tas_create(struct i2c_adapter *adapter, > struct device_node *node, > int addr) > @@ -902,7 +904,7 @@ static int tas_create(struct i2c_adapter > * Let i2c-core delete that device on driver removal. > * This is safe because i2c-core holds the core_lock mutex for us. > */ > - list_add_tail(&client->detected, &client->driver->clients); > + list_add_tail(&client->detected, &tas_driver.clients); > return 0; > } > > --- linux-2.6.32-rc1.orig/sound/ppc/keywest.c 2009-09-30 15:13:12.000000000 +0200 > +++ linux-2.6.32-rc1/sound/ppc/keywest.c 2009-09-30 15:13:58.000000000 +0200 > @@ -40,6 +40,8 @@ static int keywest_probe(struct i2c_clie > return 0; > } > > +struct i2c_driver keywest_driver; > + > /* > * This is kind of a hack, best would be to turn powermac to fixed i2c > * bus numbers and declare the sound device as part of platform > @@ -65,7 +67,7 @@ static int keywest_attach_adapter(struct > * This is safe because i2c-core holds the core_lock mutex for us. > */ > list_add_tail(&keywest_ctx->client->detected, > - &keywest_ctx->client->driver->clients); > + &keywest_driver.clients); > return 0; > } > > > > -- > Jean Delvare >
Hi Takashi, Thanks for the swift reply. On Wed, 30 Sep 2009 16:13:49 +0200, Takashi Iwai wrote: > At Wed, 30 Sep 2009 15:25:42 +0200, > Jean Delvare wrote: > > > > If i2c device probing fails, then there is no driver to dereference > > after calling i2c_new_device(). Stop assuming that probing will always > > succeed, to avoid NULL pointer dereferences. We have an easier access > > to the driver anyway. > > > > Reported-by: Tim Shepard <shep@alum.mit.edu> > > Signed-off-by: Jean Delvare <khali@linux-fr.org> > > Cc: Johannes Berg <johannes@sipsolutions.net> > > --- > > The code is similar to the one in therm_adt746x, for which Tim reported > > a real-world oops, so it should be fixed ASAP. > > Jean, thanks for the patch. > > I'm just wondering whether the additional NULL check of client->driver > would be enough? If yes, sound/aoa/onyx.c has it, at least, and we > can add the similar checks to the rest, too. The NULL check of client->driver, if followed by a call to i2c_unregister_device(), would indeed be enough. But unlike the onyx driver which we know we sometimes load erroneously, the other drivers should never fail. I am reluctant to add code to handle error cases which are not supposed to happen, which is why I prefer my proposed fix: it does not add code. That being said, I will be happy with any solution that fixes the problem, so if you prefer client->driver NULL checks, I can send a patch doing this. Thanks,
On Wed, 2009-09-30 at 17:00 +0200, Jean Delvare wrote: > The NULL check of client->driver, if followed by a call to > i2c_unregister_device(), would indeed be enough. But unlike the onyx > driver which we know we sometimes load erroneously, the other drivers > should never fail. All of these drivers can be loaded manually and then fail though, or am I misunderstanding something? johannes
On Wed, 30 Sep 2009 17:05:11 +0200, Johannes Berg wrote: > On Wed, 2009-09-30 at 17:00 +0200, Jean Delvare wrote: > > > The NULL check of client->driver, if followed by a call to > > i2c_unregister_device(), would indeed be enough. But unlike the onyx > > driver which we know we sometimes load erroneously, the other drivers > > should never fail. > > All of these drivers can be loaded manually and then fail though, or am > I misunderstanding something? I don't think so. At least tas and keywest have checks before they attempt to instantiate an i2c client, which I think are reliable. The onyx case is different because apparently some machines have it but are difficult to detect: /* if that didn't work, try desperate mode for older * machines that have stuff missing from the device tree */ And then we have to attempt to instantiate i2c devices at a not completely known address, and that may fail. I think this is the reason why onyx has this extra client->driver NULL check and the other two drivers do not. I would really love if someone with good knowledge of the device tree on mac would convert all these hacks to proper i2c device declarations. All the infrastructure is available already, but I don't know enough about open firmware and mac the device tree to do it myself.
--- linux-2.6.32-rc1.orig/sound/aoa/codecs/onyx.c 2009-09-30 15:13:12.000000000 +0200 +++ linux-2.6.32-rc1/sound/aoa/codecs/onyx.c 2009-09-30 15:13:58.000000000 +0200 @@ -996,6 +996,8 @@ static void onyx_exit_codec(struct aoa_c onyx->codec.soundbus_dev->detach_codec(onyx->codec.soundbus_dev, onyx); } +static struct i2c_driver onyx_driver; + static int onyx_create(struct i2c_adapter *adapter, struct device_node *node, int addr) @@ -1027,7 +1029,7 @@ static int onyx_create(struct i2c_adapte * Let i2c-core delete that device on driver removal. * This is safe because i2c-core holds the core_lock mutex for us. */ - list_add_tail(&client->detected, &client->driver->clients); + list_add_tail(&client->detected, &onyx_driver.clients); return 0; } --- linux-2.6.32-rc1.orig/sound/aoa/codecs/tas.c 2009-09-30 15:13:12.000000000 +0200 +++ linux-2.6.32-rc1/sound/aoa/codecs/tas.c 2009-09-30 15:13:58.000000000 +0200 @@ -882,6 +882,8 @@ static void tas_exit_codec(struct aoa_co } +static struct i2c_driver tas_driver; + static int tas_create(struct i2c_adapter *adapter, struct device_node *node, int addr) @@ -902,7 +904,7 @@ static int tas_create(struct i2c_adapter * Let i2c-core delete that device on driver removal. * This is safe because i2c-core holds the core_lock mutex for us. */ - list_add_tail(&client->detected, &client->driver->clients); + list_add_tail(&client->detected, &tas_driver.clients); return 0; } --- linux-2.6.32-rc1.orig/sound/ppc/keywest.c 2009-09-30 15:13:12.000000000 +0200 +++ linux-2.6.32-rc1/sound/ppc/keywest.c 2009-09-30 15:13:58.000000000 +0200 @@ -40,6 +40,8 @@ static int keywest_probe(struct i2c_clie return 0; } +struct i2c_driver keywest_driver; + /* * This is kind of a hack, best would be to turn powermac to fixed i2c * bus numbers and declare the sound device as part of platform @@ -65,7 +67,7 @@ static int keywest_attach_adapter(struct * This is safe because i2c-core holds the core_lock mutex for us. */ list_add_tail(&keywest_ctx->client->detected, - &keywest_ctx->client->driver->clients); + &keywest_driver.clients); return 0; }
If i2c device probing fails, then there is no driver to dereference after calling i2c_new_device(). Stop assuming that probing will always succeed, to avoid NULL pointer dereferences. We have an easier access to the driver anyway. Reported-by: Tim Shepard <shep@alum.mit.edu> Signed-off-by: Jean Delvare <khali@linux-fr.org> Cc: Johannes Berg <johannes@sipsolutions.net> --- The code is similar to the one in therm_adt746x, for which Tim reported a real-world oops, so it should be fixed ASAP. sound/aoa/codecs/onyx.c | 4 +++- sound/aoa/codecs/tas.c | 4 +++- sound/ppc/keywest.c | 4 +++- 3 files changed, 9 insertions(+), 3 deletions(-)