[alsa-devel] [PATCH] sound: Don't assume i2c device probing always succeeds
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(-)
--- 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; }
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);}
-- 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.
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.
On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
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.
This looks good to me. Please add the following comment before the client->driver check in both drivers:
/* * We know the driver is already loaded, so the device should be * already bound. If not it means binding failed, and then there * is no point in keeping the device instantiated. */
Otherwise it's a little difficult to understand why the check is there.
At Wed, 30 Sep 2009 18:55:05 +0200, Jean Delvare wrote:
On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
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.
This looks good to me. Please add the following comment before the client->driver check in both drivers:
/* * We know the driver is already loaded, so the device should be * already bound. If not it means binding failed, and then there * is no point in keeping the device instantiated. */
Otherwise it's a little difficult to understand why the check is there.
Fair enough. I applied the patch with the comment now. Thanks!
Takashi
Hi Takashi,
On Thu, 01 Oct 2009 08:52:59 +0200, Takashi Iwai wrote:
At Wed, 30 Sep 2009 18:55:05 +0200, Jean Delvare wrote:
On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
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.
This looks good to me. Please add the following comment before the client->driver check in both drivers:
/* * We know the driver is already loaded, so the device should be * already bound. If not it means binding failed, and then there * is no point in keeping the device instantiated. */
Otherwise it's a little difficult to understand why the check is there.
Fair enough. I applied the patch with the comment now. Thanks!
I see this is upstream now. While the keywest fix was essentially theoretical, the tas one addresses a crash which really could happen, so I think it would be worth sending to stable for 2.6.31. What do you think? Will you take care, or do you want me to?
Thanks,
At Sun, 4 Oct 2009 11:35:21 +0200, Jean Delvare wrote:
Hi Takashi,
On Thu, 01 Oct 2009 08:52:59 +0200, Takashi Iwai wrote:
At Wed, 30 Sep 2009 18:55:05 +0200, Jean Delvare wrote:
On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
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.
This looks good to me. Please add the following comment before the client->driver check in both drivers:
/* * We know the driver is already loaded, so the device should be * already bound. If not it means binding failed, and then there * is no point in keeping the device instantiated. */
Otherwise it's a little difficult to understand why the check is there.
Fair enough. I applied the patch with the comment now. Thanks!
I see this is upstream now. While the keywest fix was essentially theoretical, the tas one addresses a crash which really could happen, so I think it would be worth sending to stable for 2.6.31. What do you think? Will you take care, or do you want me to?
Agreed, it's safer to send the patch to stable tree. I'm going to submit it.
thanks,
Takashi
participants (3)
-
Jean Delvare -
Johannes Berg -
Takashi Iwai