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@xxxxxxxxxxxx>
> > > Signed-off-by: Jean Delvare <khali@xxxxxxxxxxxx>
> > > Cc: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> > > ---
> > > 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
