[alsa-devel] [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL
The callers use the return value of max98088_get_channel as array index to access max98088->dai[] array. Add BUG() assertion for out of bound access of max98088->dai[] array.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/max98088.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c index ac65a2d..aaca91c 100644 --- a/sound/soc/codecs/max98088.c +++ b/sound/soc/codecs/max98088.c @@ -1703,6 +1703,7 @@ static int max98088_get_channel(const char *name) return 0; if (strcmp(name, "EQ2 Mode") == 0) return 1; + BUG(); return -EINVAL; }
The callers use the return value of max98095_get_bq_channel as array index to access max98095->dai[] array. Add BUG() assertion for out of bound access of max98095->dai[] array.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/max98095.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c index 668434d..973c02d 100644 --- a/sound/soc/codecs/max98095.c +++ b/sound/soc/codecs/max98095.c @@ -1998,6 +1998,7 @@ static int max98095_get_bq_channel(const char *name) return 0; if (strcmp(name, "Biquad2 Mode") == 0) return 1; + BUG(); return -EINVAL; }
On 29/09/11 00:02, Axel Lin wrote:
The callers use the return value of max98095_get_bq_channel as array index to access max98095->dai[] array. Add BUG() assertion for out of bound access of max98095->dai[] array.
Same here, fix the problem in the callers.
---- Check the return value of max98095_get_bq_channel in the callers and propagate any errors up. Remove the BUG_ON(channel > 1) since max98095_get_bq_channel never returns a value larger than 1.
Signed-off-by: Ryan Mallon rmallon@gmail.com ---
diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c index 668434d..55eccea 100644 --- a/sound/soc/codecs/max98095.c +++ b/sound/soc/codecs/max98095.c @@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol, int fs, best, best_val, i; int regmask, regsave;
- BUG_ON(channel > 1); + if (channel < 0) + return channel;
if (!pdata || !max98095->bq_textcnt) return 0; @@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol, int channel = max98095_get_bq_channel(kcontrol->id.name); struct max98095_cdata *cdata;
+ if (channel < 0) + return channel; + cdata = &max98095->dai[channel]; ucontrol->value.enumerated.item[0] = cdata->bq_sel;
On Thu, Sep 29, 2011 at 7:19 AM, Ryan Mallon rmallon@gmail.com wrote:
On 29/09/11 00:02, Axel Lin wrote:
The callers use the return value of max98095_get_bq_channel as array index to access max98095->dai[] array. Add BUG() assertion for out of bound access of max98095->dai[] array.
Same here, fix the problem in the callers.
Check the return value of max98095_get_bq_channel in the callers and propagate any errors up. Remove the BUG_ON(channel > 1) since max98095_get_bq_channel never returns a value larger than 1.
Signed-off-by: Ryan Mallon rmallon@gmail.com
diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c index 668434d..55eccea 100644 --- a/sound/soc/codecs/max98095.c +++ b/sound/soc/codecs/max98095.c @@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol, int fs, best, best_val, i; int regmask, regsave;
- BUG_ON(channel > 1);
- if (channel < 0)
- return channel;
If use BUG() happens in max98095_get_bq_channel, it will not return here?
if (!pdata || !max98095->bq_textcnt) return 0; @@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol, int channel = max98095_get_bq_channel(kcontrol->id.name); struct max98095_cdata *cdata;
- if (channel < 0)
- return channel;
cdata = &max98095->dai[channel]; ucontrol->value.enumerated.item[0] = cdata->bq_sel;
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 29/09/11 11:35, Dave Young wrote:
On Thu, Sep 29, 2011 at 7:19 AM, Ryan Mallon rmallon@gmail.com wrote:
On 29/09/11 00:02, Axel Lin wrote:
The callers use the return value of max98095_get_bq_channel as array index to access max98095->dai[] array. Add BUG() assertion for out of bound access of max98095->dai[] array.
Same here, fix the problem in the callers.
Check the return value of max98095_get_bq_channel in the callers and propagate any errors up. Remove the BUG_ON(channel > 1) since max98095_get_bq_channel never returns a value larger than 1.
Signed-off-by: Ryan Mallon rmallon@gmail.com
diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c index 668434d..55eccea 100644 --- a/sound/soc/codecs/max98095.c +++ b/sound/soc/codecs/max98095.c @@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol, int fs, best, best_val, i; int regmask, regsave;
BUG_ON(channel > 1);
if (channel < 0)return channel;If use BUG() happens in max98095_get_bq_channel, it will not return here?
Not quite sure what you mean?
If CONFIG_BUG was not enabled for the original version, then it would not return at the BUG_ON and would either crash or cause odd behaviour if it tried to index channel as -1.
My patch is removing the BUG_ON and replacing it with a proper check and return. It doesn't need to check > 1 since max98095_get_bq_channel never returns that.
My understanding is that device drivers, in general, should not call BUG. BUG is for unrecoverable errors which leave the kernel in some unstable state. Here we can just return an error code.
~Ryan
if (!pdata || !max98095->bq_textcnt) return 0;@@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol, int channel = max98095_get_bq_channel(kcontrol->id.name); struct max98095_cdata *cdata;
if (channel < 0)return channel;cdata = &max98095->dai[channel]; ucontrol->value.enumerated.item[0] = cdata->bq_sel;-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Thu, Sep 29, 2011 at 9:52 AM, Ryan Mallon rmallon@gmail.com wrote:
On 29/09/11 11:35, Dave Young wrote:
On Thu, Sep 29, 2011 at 7:19 AM, Ryan Mallon rmallon@gmail.com wrote:
On 29/09/11 00:02, Axel Lin wrote:
The callers use the return value of max98095_get_bq_channel as array index to access max98095->dai[] array. Add BUG() assertion for out of bound access of max98095->dai[] array.
Same here, fix the problem in the callers.
Check the return value of max98095_get_bq_channel in the callers and propagate any errors up. Remove the BUG_ON(channel > 1) since max98095_get_bq_channel never returns a value larger than 1.
Signed-off-by: Ryan Mallon rmallon@gmail.com
diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c index 668434d..55eccea 100644 --- a/sound/soc/codecs/max98095.c +++ b/sound/soc/codecs/max98095.c @@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol, int fs, best, best_val, i; int regmask, regsave;
- BUG_ON(channel > 1);
- if (channel < 0)
- return channel;
If use BUG() happens in max98095_get_bq_channel, it will not return here?
Not quite sure what you mean?
I means if Axel Lin's patch applied, and CONFIG_BUG is on, it will panic firstly the if condition will be never entered.
If CONFIG_BUG was not enabled for the original version, then it would not return at the BUG_ON and would either crash or cause odd behaviour if it tried to index channel as -1.
My patch is removing the BUG_ON and replacing it with a proper check and return. It doesn't need to check > 1 since max98095_get_bq_channel never returns that.
My understanding is that device drivers, in general, should not call BUG. BUG is for unrecoverable errors which leave the kernel in some unstable state. Here we can just return an error code.
Agree
~Ryan
if (!pdata || !max98095->bq_textcnt) return 0; @@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol, int channel = max98095_get_bq_channel(kcontrol->id.name); struct max98095_cdata *cdata;
- if (channel < 0)
- return channel;
cdata = &max98095->dai[channel]; ucontrol->value.enumerated.item[0] = cdata->bq_sel;
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 29/09/11 11:59, Dave Young wrote:
On Thu, Sep 29, 2011 at 9:52 AM, Ryan Mallon rmallon@gmail.com wrote:
On 29/09/11 11:35, Dave Young wrote:
On Thu, Sep 29, 2011 at 7:19 AM, Ryan Mallon rmallon@gmail.com wrote:
On 29/09/11 00:02, Axel Lin wrote:
The callers use the return value of max98095_get_bq_channel as array index to access max98095->dai[] array. Add BUG() assertion for out of bound access of max98095->dai[] array.
Same here, fix the problem in the callers.
Check the return value of max98095_get_bq_channel in the callers and propagate any errors up. Remove the BUG_ON(channel > 1) since max98095_get_bq_channel never returns a value larger than 1.
Signed-off-by: Ryan Mallon rmallon@gmail.com
diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c index 668434d..55eccea 100644 --- a/sound/soc/codecs/max98095.c +++ b/sound/soc/codecs/max98095.c @@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol, int fs, best, best_val, i; int regmask, regsave;
BUG_ON(channel > 1);
if (channel < 0)return channel;If use BUG() happens in max98095_get_bq_channel, it will not return here?
Not quite sure what you mean?
I means if Axel Lin's patch applied, and CONFIG_BUG is on, it will panic firstly the if condition will be never entered.
My patch is a replacement for Axel's patch, not on top of it. For Axel's patch it would panic if channel was less than zero if CONFIG_BUG was enabled, but would still have undefined behaviour if CONFIG_BUG was not enabled.
~Ryan
If CONFIG_BUG was not enabled for the original version, then it would not return at the BUG_ON and would either crash or cause odd behaviour if it tried to index channel as -1.
My patch is removing the BUG_ON and replacing it with a proper check and return. It doesn't need to check > 1 since max98095_get_bq_channel never returns that.
My understanding is that device drivers, in general, should not call BUG. BUG is for unrecoverable errors which leave the kernel in some unstable state. Here we can just return an error code.
Agree
~Ryan
if (!pdata || !max98095->bq_textcnt) return 0;@@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol, int channel = max98095_get_bq_channel(kcontrol->id.name); struct max98095_cdata *cdata;
if (channel < 0)return channel;cdata = &max98095->dai[channel]; ucontrol->value.enumerated.item[0] = cdata->bq_sel;-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Thu, Sep 29, 2011 at 10:01 AM, Ryan Mallon rmallon@gmail.com wrote:
On 29/09/11 11:59, Dave Young wrote:
On Thu, Sep 29, 2011 at 9:52 AM, Ryan Mallon rmallon@gmail.com wrote:
On 29/09/11 11:35, Dave Young wrote:
On Thu, Sep 29, 2011 at 7:19 AM, Ryan Mallon rmallon@gmail.com wrote:
On 29/09/11 00:02, Axel Lin wrote:
The callers use the return value of max98095_get_bq_channel as array index to access max98095->dai[] array. Add BUG() assertion for out of bound access of max98095->dai[] array.
Same here, fix the problem in the callers.
Check the return value of max98095_get_bq_channel in the callers and propagate any errors up. Remove the BUG_ON(channel > 1) since max98095_get_bq_channel never returns a value larger than 1.
Signed-off-by: Ryan Mallon rmallon@gmail.com
diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c index 668434d..55eccea 100644 --- a/sound/soc/codecs/max98095.c +++ b/sound/soc/codecs/max98095.c @@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol, int fs, best, best_val, i; int regmask, regsave;
- BUG_ON(channel > 1);
- if (channel < 0)
- return channel;
If use BUG() happens in max98095_get_bq_channel, it will not return here?
Not quite sure what you mean?
I means if Axel Lin's patch applied, and CONFIG_BUG is on, it will panic firstly the if condition will be never entered.
My patch is a replacement for Axel's patch, not on top of it. For Axel's patch it would panic if channel was less than zero if CONFIG_BUG was enabled, but would still have undefined behaviour if CONFIG_BUG was not enabled.
So that's good, thanks
~Ryan
If CONFIG_BUG was not enabled for the original version, then it would not return at the BUG_ON and would either crash or cause odd behaviour if it tried to index channel as -1.
My patch is removing the BUG_ON and replacing it with a proper check and return. It doesn't need to check > 1 since max98095_get_bq_channel never returns that.
My understanding is that device drivers, in general, should not call BUG. BUG is for unrecoverable errors which leave the kernel in some unstable state. Here we can just return an error code.
Agree
~Ryan
if (!pdata || !max98095->bq_textcnt) return 0; @@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol, int channel = max98095_get_bq_channel(kcontrol->id.name); struct max98095_cdata *cdata;
- if (channel < 0)
- return channel;
cdata = &max98095->dai[channel]; ucontrol->value.enumerated.item[0] = cdata->bq_sel;
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, Sep 28, 2011 at 10:02 PM, Axel Lin axel.lin@gmail.com wrote:
The callers use the return value of max98095_get_bq_channel as array index to access max98095->dai[] array. Add BUG() assertion for out of bound access of max98095->dai[] array.
Signed-off-by: Axel Lin axel.lin@gmail.com
sound/soc/codecs/max98095.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c index 668434d..973c02d 100644 --- a/sound/soc/codecs/max98095.c +++ b/sound/soc/codecs/max98095.c @@ -1998,6 +1998,7 @@ static int max98095_get_bq_channel(const char *name) return 0; if (strcmp(name, "Biquad2 Mode") == 0) return 1;
- BUG();
return -EINVAL;
below better? BUG_ON(strcmp(name, "Biquad2 Mode")); return 1;
Or BUG_ON(channel < 0) in caller
}
-- 1.7.4.1
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 29/09/11 00:01, Axel Lin wrote:
The callers use the return value of max98088_get_channel as array index to access max98088->dai[] array. Add BUG() assertion for out of bound access of max98088->dai[] array.
BUG() is pretty heavy handed for a driver. Why not fix the problem properly in the callers?
---- Check the return value of max98088_get_channel in the callers and propagate any errors up.
Signed-off-by: Ryan Mallon rmallon@gmail.com ---
diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c index ac65a2d..e07700e 100644 --- a/sound/soc/codecs/max98088.c +++ b/sound/soc/codecs/max98088.c @@ -1811,6 +1811,9 @@ static int max98088_put_eq_enum(struct snd_kcontrol *kcontrol, struct max98088_cdata *cdata; int sel = ucontrol->value.integer.value[0];
+ if (channel < 0) + return channel; + cdata = &max98088->dai[channel];
if (sel >= pdata->eq_cfgcnt) @@ -1838,6 +1841,9 @@ static int max98088_get_eq_enum(struct snd_kcontrol *kcontrol, int channel = max98088_get_channel(kcontrol->id.name); struct max98088_cdata *cdata;
+ if (channel < 0) + return channel; + cdata = &max98088->dai[channel]; ucontrol->value.enumerated.item[0] = cdata->eq_sel; return 0;
On Thu, Sep 29, 2011 at 09:15:03AM +1000, Ryan Mallon wrote:
On 29/09/11 00:01, Axel Lin wrote:
The callers use the return value of max98088_get_channel as array index to access max98088->dai[] array. Add BUG() assertion for out of bound access of max98088->dai[] array.
BUG() is pretty heavy handed for a driver. Why not fix the problem properly in the callers?
There's nothing constructive that any of the callers can do with an error code - it's a clear bug in something (probably the driver) if we get called for a bad control. Simply returning an error code isn't terribly helpful, it's very obscure what's gone wrong and why. We at least need a log message.
On 29/09/11 20:34, Mark Brown wrote:
On Thu, Sep 29, 2011 at 09:15:03AM +1000, Ryan Mallon wrote:
On 29/09/11 00:01, Axel Lin wrote:
The callers use the return value of max98088_get_channel as array index to access max98088->dai[] array. Add BUG() assertion for out of bound access of max98088->dai[] array.
BUG() is pretty heavy handed for a driver. Why not fix the problem properly in the callers?
There's nothing constructive that any of the callers can do with an error code - it's a clear bug in something (probably the driver) if we get called for a bad control. Simply returning an error code isn't terribly helpful, it's very obscure what's gone wrong and why. We at least need a log message.
Yeah, it can basically only happen if there is a mismatch between the kcontrol definition and the get_channel function in the driver. Would you be happy with adding a:
dev_err(codec->dev, "Bad kcontrol channel name\n");
and then returning the error? It doesn't seem worth panicking the whole driver/system for a bug like this.
~Ryan
On 29/09/11 20:34, Mark Brown wrote:
On Thu, Sep 29, 2011 at 09:15:03AM +1000, Ryan Mallon wrote:
On 29/09/11 00:01, Axel Lin wrote:
The callers use the return value of max98088_get_channel as array index to access max98088->dai[] array. Add BUG() assertion for out of bound access of max98088->dai[] array.
BUG() is pretty heavy handed for a driver. Why not fix the problem properly in the callers?
There's nothing constructive that any of the callers can do with an error code - it's a clear bug in something (probably the driver) if we get called for a bad control. Simply returning an error code isn't terribly helpful, it's very obscure what's gone wrong and why. We at least need a log message.
How about something like this (untested) patch? --- Make the max98088 codec controls[] array a static global and iterate over it in max98088_get_channel rather than duplicating the hardcoded strings. Add a warning if the channel name is not found and check for the error value in the callers.
Signed-off-by: Ryan Mallon rmallon@gmail.com ---
diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c index ac65a2d..bc2b922 100644 --- a/sound/soc/codecs/max98088.c +++ b/sound/soc/codecs/max98088.c @@ -1697,13 +1697,28 @@ static struct snd_soc_dai_driver max98088_dai[] = { } };
-static int max98088_get_channel(const char *name) +static struct snd_kcontrol_new controls[] = { + SOC_ENUM_EXT("EQ1 Mode", + max98088->eq_enum, + max98088_get_eq_enum, + max98088_put_eq_enum), + SOC_ENUM_EXT("EQ2 Mode", + max98088->eq_enum, + max98088_get_eq_enum, + max98088_put_eq_enum), +}; + +static int max98088_get_channel(struct snd_soc_codec *codec, const char *name) { - if (strcmp(name, "EQ1 Mode") == 0) - return 0; - if (strcmp(name, "EQ2 Mode") == 0) - return 1; - return -EINVAL; + int i; + + for (i = 0; i < ARRAY_SIZE(controls); i++) + if (strcmp(name, controls[i].name) == 0) + return i; + + /* Shouldn't happen */ + dev_err(codec->dev, "Bad channel name '%s'\n", name); + return -EINVAL; }
static void max98088_setup_eq1(struct snd_soc_codec *codec) @@ -1807,10 +1822,13 @@ static int max98088_put_eq_enum(struct snd_kcontrol *kcontrol, struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); struct max98088_priv *max98088 = snd_soc_codec_get_drvdata(codec); struct max98088_pdata *pdata = max98088->pdata; - int channel = max98088_get_channel(kcontrol->id.name); + int channel = max98088_get_channel(codec, kcontrol->id.name); struct max98088_cdata *cdata; int sel = ucontrol->value.integer.value[0];
+ if (channel < 0) + return channel; + cdata = &max98088->dai[channel];
if (sel >= pdata->eq_cfgcnt) @@ -1835,9 +1853,12 @@ static int max98088_get_eq_enum(struct snd_kcontrol *kcontrol, { struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); struct max98088_priv *max98088 = snd_soc_codec_get_drvdata(codec); - int channel = max98088_get_channel(kcontrol->id.name); + int channel = max98088_get_channel(codec, kcontrol->id.name); struct max98088_cdata *cdata;
+ if (channel < 0) + return channel; + cdata = &max98088->dai[channel]; ucontrol->value.enumerated.item[0] = cdata->eq_sel; return 0; @@ -1853,17 +1874,6 @@ static void max98088_handle_eq_pdata(struct snd_soc_codec *codec) const char **t; int ret;
- struct snd_kcontrol_new controls[] = { - SOC_ENUM_EXT("EQ1 Mode", - max98088->eq_enum, - max98088_get_eq_enum, - max98088_put_eq_enum), - SOC_ENUM_EXT("EQ2 Mode", - max98088->eq_enum, - max98088_get_eq_enum, - max98088_put_eq_enum), - }; - cfg = pdata->eq_cfg; cfgcnt = pdata->eq_cfgcnt;
participants (4)
-
Axel Lin -
Dave Young -
Mark Brown -
Ryan Mallon