wm8994: range checking issue

RFC PATCH In review Dan Carpenter 2 participants 5 messages Mar 24, 2010 – Mar 25, 2010

Subsystems (4)

Sound Kernel janitors ASoC Wolfson microelectronics drivers

lore

Share & tools

lore

Dan Carpenter

author

2010-03-24 12:01 UTC #0 lore raw

RFC PATCH 1/1 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

sound/soc/codecs/wm8994.c +7 -5

index 29f3771..d9c179a 100644
@@ -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;

Mark Brown

2010-03-24 12:59 UTC #1 lore raw

Quoted 4 lines · Dan Carpenter

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.

Quoted 4 lines

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.

Dan Carpenter

author

2010-03-24 14:06 UTC #2 lore raw

Quoted 14 lines · Mark Brown

Quoted 4 lines · Dan Carpenter
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.
Quoted 4 lines
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.

Quoted 4 lines

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

Mark Brown

2010-03-24 14:31 UTC #3 lore raw

Quoted 10 lines · Dan Carpenter

Quoted 4 lines · Mark Brown
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).

Quoted 1 line

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.

Dan Carpenter

author

2010-03-25 10:58 UTC #4 lore raw

Quoted 23 lines · Mark Brown

Quoted 10 lines · Dan Carpenter
Quoted 4 lines · Mark Brown
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).

Quoted 9 lines

Quoted 1 line
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

Quoted 1 line

I'll send a patch for _MAX_CACHED_REGISTER later today.