At Wed, 30 Sep 2009 17:00:06 +0200, Jean Delvare wrote:
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.
Yes, indeed I prefer NULL check because the user can know the error at the right place. I share your concern about the code addition, though :)
I already made a patch below, but it's totally untested. It'd be helpful if someone can do review and build-test it.
thanks,
Takashi
--- diff --git a/sound/aoa/codecs/tas.c b/sound/aoa/codecs/tas.c index f0ebc97..0f810c8 100644 --- a/sound/aoa/codecs/tas.c +++ b/sound/aoa/codecs/tas.c @@ -897,6 +897,10 @@ static int tas_create(struct i2c_adapter *adapter, client = i2c_new_device(adapter, &info); if (!client) return -ENODEV; + if (!client->driver) { + i2c_unregister_device(client); + return -ENODEV; + }
/* * Let i2c-core delete that device on driver removal. diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c index 835fa19..473c5a6 100644 --- a/sound/ppc/keywest.c +++ b/sound/ppc/keywest.c @@ -59,6 +59,13 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter) strlcpy(info.type, "keywest", I2C_NAME_SIZE); info.addr = keywest_ctx->addr; keywest_ctx->client = i2c_new_device(adapter, &info); + if (!keywest_ctx->client) + return -ENODEV; + if (!keywest_ctx->client->driver) { + i2c_unregister_device(keywest_ctx->client); + keywest_ctx->client = NULL; + return -ENODEV; + } /* * Let i2c-core delete that device on driver removal.