[alsa-devel] [rfc patch] wm8994: range checking issue
Smatch complained about BUG_ON(reg > WM8994_MAX_REGISTER) because the actual number of elements in the array was WM8994_REG_CACHE_SIZE + 1.
I changed the BUG_ON() to return -EINVAL.
I was confused why WM8994_REG_CACHE_SIZE was different from the actual size of ->reg_cache and I was concerned because some places used ARRAY_SIZE() to find the end of the array and other places used WM8994_REG_CACHE_SIZE. In my patch, I made them the same.
I don't have the hardware to test this and some of this patch is just guess work.
For example, I left it in, but why is there a -1 here? 3711 /* Fill the cache with physical values we inherited; don't reset */ 3712 ret = wm8994_bulk_read(codec->control_data, 0, 3713 ARRAY_SIZE(wm8994->reg_cache) - 1, 3714 codec->reg_cache);
I didn't sign this off because I figured I'd probably need to send a second version after I hear the feed back.
regards, dan carpenter
diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c index 29f3771..d9c179a 100644 --- a/sound/soc/codecs/wm8994.c +++ b/sound/soc/codecs/wm8994.c @@ -59,13 +59,13 @@ static int wm8994_retune_mobile_base[] = { WM8994_AIF2_EQ_GAINS_1, };
-#define WM8994_REG_CACHE_SIZE 0x621 +#define WM8994_REG_CACHE_SIZE 0x622
/* codec private data */ struct wm8994_priv { struct wm_hubs_data hubs; struct snd_soc_codec codec; - u16 reg_cache[WM8994_REG_CACHE_SIZE + 1]; + u16 reg_cache[WM8994_REG_CACHE_SIZE]; int sysclk[2]; int sysclk_rate[2]; int mclk[2]; @@ -1697,7 +1697,8 @@ static int wm8994_write(struct snd_soc_codec *codec, unsigned int reg, { struct wm8994_priv *wm8994 = codec->private_data;
- BUG_ON(reg > WM8994_MAX_REGISTER); + if (reg >= WM8994_REG_CACHE_SIZE) + return -EINVAL;
if (!wm8994_volatile(reg)) wm8994->reg_cache[reg] = value; @@ -1710,7 +1711,8 @@ static unsigned int wm8994_read(struct snd_soc_codec *codec, { u16 *reg_cache = codec->reg_cache;
- BUG_ON(reg > WM8994_MAX_REGISTER); + if (reg >= WM8994_REG_CACHE_SIZE) + return -EINVAL;
if (wm8994_volatile(reg)) return wm8994_reg_read(codec->control_data, reg); @@ -3700,7 +3702,7 @@ static int wm8994_codec_probe(struct platform_device *pdev) codec->set_bias_level = wm8994_set_bias_level; codec->dai = &wm8994_dai[0]; codec->num_dai = 3; - codec->reg_cache_size = WM8994_MAX_REGISTER; + codec->reg_cache_size = WM8994_REG_CACHE_SIZE; codec->reg_cache = &wm8994->reg_cache; codec->dev = &pdev->dev;
On Wed, Mar 24, 2010 at 03:01:07PM +0300, Dan Carpenter wrote:
Smatch complained about BUG_ON(reg > WM8994_MAX_REGISTER) because the actual number of elements in the array was WM8994_REG_CACHE_SIZE + 1.
I changed the BUG_ON() to return -EINVAL.
Please don't introduce orthogonal changes like this in patches, it's bad practice and increases the chances of your patch being nacked.
I was confused why WM8994_REG_CACHE_SIZE was different from the actual size of ->reg_cache and I was concerned because some places used ARRAY_SIZE() to find the end of the array and other places used WM8994_REG_CACHE_SIZE. In my patch, I made them the same.
This is caused by confusion with the MAX_CACHED_REGISTER definition in the header. Best to use that one consistently, I guess - I've got a sneaking suspicion something has gone AWOL in the driver publication process.
On Wed, Mar 24, 2010 at 12:59:46PM +0000, Mark Brown wrote:
On Wed, Mar 24, 2010 at 03:01:07PM +0300, Dan Carpenter wrote:
Smatch complained about BUG_ON(reg > WM8994_MAX_REGISTER) because the actual number of elements in the array was WM8994_REG_CACHE_SIZE + 1.
I changed the BUG_ON() to return -EINVAL.
Please don't introduce orthogonal changes like this in patches, it's bad practice and increases the chances of your patch being nacked.
I was confused why WM8994_REG_CACHE_SIZE was different from the actual size of ->reg_cache and I was concerned because some places used ARRAY_SIZE() to find the end of the array and other places used WM8994_REG_CACHE_SIZE. In my patch, I made them the same.
This is caused by confusion with the MAX_CACHED_REGISTER definition in the header. Best to use that one consistently, I guess - I've got a sneaking suspicion something has gone AWOL in the driver publication process.
Hm... That sounds more involved than I anticipated. I don't have the hardware and don't feel comfortable making complicated changes if I can't test them.
Can someone else take care of this.
regards, dan carpenter
On Wed, Mar 24, 2010 at 05:06:21PM +0300, Dan Carpenter wrote:
On Wed, Mar 24, 2010 at 12:59:46PM +0000, Mark Brown wrote:
This is caused by confusion with the MAX_CACHED_REGISTER definition in the header. Best to use that one consistently, I guess - I've got a sneaking suspicion something has gone AWOL in the driver publication process.
Hm... That sounds more involved than I anticipated. I don't have the hardware and don't feel comfortable making complicated changes if I can't test them.
Not really, it's just a case of picking the value to standardise on for the size of the array instead of the one you picked. However, now I look at it again REG_CACHE_SIZE is the one we want and _MAX_CACHED_REGISTER is bitrot which should be removed.
I didn't look as closely as I might due to the extraneous changes for BUG_ON() I mentioned which meant the patch wouldn't be applied anyway. Those shouldn't be changed because there's no way anything in the kernel should be generating a reference to a register which doesn't physically exist (which is what they check for).
Can someone else take care of this.
Actually, now I look even more closely there's further issues with the patch - you're missing the fact that the register cache is only used for non-volatile registers but all registers beyond the end of the register cache are treated as volatile. This means that I'm not convinced there are any actual problems here, I'm not sure what analysis smatch is doing but it looks to have generated false positives here.
I'll send a patch for _MAX_CACHED_REGISTER later today.
On Wed, Mar 24, 2010 at 02:31:39PM +0000, Mark Brown wrote:
On Wed, Mar 24, 2010 at 05:06:21PM +0300, Dan Carpenter wrote:
On Wed, Mar 24, 2010 at 12:59:46PM +0000, Mark Brown wrote:
This is caused by confusion with the MAX_CACHED_REGISTER definition in the header. Best to use that one consistently, I guess - I've got a sneaking suspicion something has gone AWOL in the driver publication process.
Hm... That sounds more involved than I anticipated. I don't have the hardware and don't feel comfortable making complicated changes if I can't test them.
Not really, it's just a case of picking the value to standardise on for the size of the array instead of the one you picked. However, now I look at it again REG_CACHE_SIZE is the one we want and _MAX_CACHED_REGISTER is bitrot which should be removed.
I didn't look as closely as I might due to the extraneous changes for BUG_ON() I mentioned which meant the patch wouldn't be applied anyway. Those shouldn't be changed because there's no way anything in the kernel should be generating a reference to a register which doesn't physically exist (which is what they check for).
Can someone else take care of this.
Actually, now I look even more closely there's further issues with the patch - you're missing the fact that the register cache is only used for non-volatile registers but all registers beyond the end of the register cache are treated as volatile. This means that I'm not convinced there are any actual problems here, I'm not sure what analysis smatch is doing but it looks to have generated false positives here.
Yup. You are right, this is a false positive. I'm very sorry about that, I misread the code as well.
The problem is that Smatch doesn't do cross function analysis yet. :/
regards, dan carpenter
I'll send a patch for _MAX_CACHED_REGISTER later today.
participants (2)
-
Dan Carpenter -
Mark Brown