[rfc patch] wm8994: range checking issue

[rfc patch] wm8994: range checking issue

[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.

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;
 

Re: [rfc patch] wm8994: range checking issue

[Mark Brown]

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.

Re: [rfc patch] wm8994: range checking issue

[Dan Carpenter]

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

Re: [rfc patch] wm8994: range checking issue

[Mark Brown]

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.

Re: [rfc patch] wm8994: range checking issue

[Dan Carpenter]

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.