diff mbox series

iio: filter: admv8818: fix out-of-bounds read in __admv8818_read_[h|l]pf_freq()

Message ID 20220922115848.1800021-1-weiyongjun@huaweicloud.com (mailing list archive)
State Accepted
Headers show
Series iio: filter: admv8818: fix out-of-bounds read in __admv8818_read_[h|l]pf_freq() | expand

Commit Message

Wei Yongjun Sept. 22, 2022, 11:58 a.m. UTC 
From: Wei Yongjun <weiyongjun1@huawei.com>

ADMV8818_SW_IN_WR0_MSK and ADMV8818_SW_OUT_WR0_MSK have 3 bits,
which means a length of 8, but freq_range_hpf and freq_range_lpf
array size is 4, may end up reading 4 elements beyond the end of
those arrays.

Fix to check value first before access freq_range_hpf and
freq_range_lpf.

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
 drivers/iio/filter/admv8818.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jonathan Cameron Sept. 24, 2022, 4:53 p.m. UTC | #1 
On Thu, 22 Sep 2022 11:58:48 +0000
Wei Yongjun <weiyongjun@huaweicloud.com> wrote:

> From: Wei Yongjun <weiyongjun1@huawei.com>
> 
> ADMV8818_SW_IN_WR0_MSK and ADMV8818_SW_OUT_WR0_MSK have 3 bits,
> which means a length of 8, but freq_range_hpf and freq_range_lpf
> array size is 4, may end up reading 4 elements beyond the end of
> those arrays.
> 
> Fix to check value first before access freq_range_hpf and
> freq_range_lpf.
> 
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

The datasheet isn't clear on whether the register simply can't take values
above b100 or that the behaviour if it does is undefined.  It would take someone
to poke the registers on a real device to find out.  Either way, this seems to
be a reasonable hardening against corrupt data back from the device.

I will leave it on list a while longer though so it is probably 6.2 material
now.

Thanks,

Jonathan

> ---
>  drivers/iio/filter/admv8818.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/filter/admv8818.c b/drivers/iio/filter/admv8818.c
> index 68de45fe21b4..fe8d46cb7f1d 100644
> --- a/drivers/iio/filter/admv8818.c
> +++ b/drivers/iio/filter/admv8818.c
> @@ -265,7 +265,7 @@ static int __admv8818_read_hpf_freq(struct admv8818_state *st, u64 *hpf_freq)
>  		return ret;
>  
>  	hpf_band = FIELD_GET(ADMV8818_SW_IN_WR0_MSK, data);
> -	if (!hpf_band) {
> +	if (!hpf_band || hpf_band > 4) {
>  		*hpf_freq = 0;
>  		return ret;
>  	}
> @@ -303,7 +303,7 @@ static int __admv8818_read_lpf_freq(struct admv8818_state *st, u64 *lpf_freq)
>  		return ret;
>  
>  	lpf_band = FIELD_GET(ADMV8818_SW_OUT_WR0_MSK, data);
> -	if (!lpf_band) {
> +	if (!lpf_band || lpf_band > 4) {
>  		*lpf_freq = 0;
>  		return ret;
>  	}
Miclaus, Antoniu Sept. 26, 2022, 6:20 a.m. UTC | #2 
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, September 24, 2022 7:54 PM
> To: Wei Yongjun <weiyongjun@huaweicloud.com>
> Cc: linux-iio@vger.kernel.org; Wei Yongjun <weiyongjun1@huawei.com>;
> Miclaus, Antoniu <Antoniu.Miclaus@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH] iio: filter: admv8818: fix out-of-bounds read in
> __admv8818_read_[h|l]pf_freq()
> 
> [External]
> 
> On Thu, 22 Sep 2022 11:58:48 +0000
> Wei Yongjun <weiyongjun@huaweicloud.com> wrote:
> 
> > From: Wei Yongjun <weiyongjun1@huawei.com>
> >
> > ADMV8818_SW_IN_WR0_MSK and ADMV8818_SW_OUT_WR0_MSK have 3
> bits,
> > which means a length of 8, but freq_range_hpf and freq_range_lpf
> > array size is 4, may end up reading 4 elements beyond the end of
> > those arrays.
> >
> > Fix to check value first before access freq_range_hpf and
> > freq_range_lpf.
> >
> > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Reviewed-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> 
> The datasheet isn't clear on whether the register simply can't take values
> above b100 or that the behaviour if it does is undefined.  It would take someone
> to poke the registers on a real device to find out.  Either way, this seems to
> be a reasonable hardening against corrupt data back from the device.
> 
> I will leave it on list a while longer though so it is probably 6.2 material
> now.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/filter/admv8818.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/filter/admv8818.c b/drivers/iio/filter/admv8818.c
> > index 68de45fe21b4..fe8d46cb7f1d 100644
> > --- a/drivers/iio/filter/admv8818.c
> > +++ b/drivers/iio/filter/admv8818.c
> > @@ -265,7 +265,7 @@ static int __admv8818_read_hpf_freq(struct
> admv8818_state *st, u64 *hpf_freq)
> >  		return ret;
> >
> >  	hpf_band = FIELD_GET(ADMV8818_SW_IN_WR0_MSK, data);
> > -	if (!hpf_band) {
> > +	if (!hpf_band || hpf_band > 4) {
> >  		*hpf_freq = 0;
> >  		return ret;
> >  	}
> > @@ -303,7 +303,7 @@ static int __admv8818_read_lpf_freq(struct
> admv8818_state *st, u64 *lpf_freq)
> >  		return ret;
> >
> >  	lpf_band = FIELD_GET(ADMV8818_SW_OUT_WR0_MSK, data);
> > -	if (!lpf_band) {
> > +	if (!lpf_band || lpf_band > 4) {
> >  		*lpf_freq = 0;
> >  		return ret;
> >  	}
Jonathan Cameron Oct. 2, 2022, 11:28 a.m. UTC | #3 
On Mon, 26 Sep 2022 06:20:20 +0000
"Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, September 24, 2022 7:54 PM
> > To: Wei Yongjun <weiyongjun@huaweicloud.com>
> > Cc: linux-iio@vger.kernel.org; Wei Yongjun <weiyongjun1@huawei.com>;
> > Miclaus, Antoniu <Antoniu.Miclaus@analog.com>; Lars-Peter Clausen
> > <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>
> > Subject: Re: [PATCH] iio: filter: admv8818: fix out-of-bounds read in
> > __admv8818_read_[h|l]pf_freq()
> > 
> > [External]
> > 
> > On Thu, 22 Sep 2022 11:58:48 +0000
> > Wei Yongjun <weiyongjun@huaweicloud.com> wrote:
> >   
> > > From: Wei Yongjun <weiyongjun1@huawei.com>
> > >
> > > ADMV8818_SW_IN_WR0_MSK and ADMV8818_SW_OUT_WR0_MSK have 3  
> > bits,  
> > > which means a length of 8, but freq_range_hpf and freq_range_lpf
> > > array size is 4, may end up reading 4 elements beyond the end of
> > > those arrays.
> > >
> > > Fix to check value first before access freq_range_hpf and
> > > freq_range_lpf.
> > >
> > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>  
> Reviewed-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

I tweaked the commit message a little to have this as hardening rather than 
a fix.  I'm fine with it being backported if that makes sense, but I don't
want it automatically picked up.

Good to harden this though.

Applied to the togreg branch of iio.git. I'll only be pushing that out as
testing until I can rebase on rc1.  In the meantime we should get 0-day
checking it etc.

Thanks,

Jonathan

> > 
> > The datasheet isn't clear on whether the register simply can't take values
> > above b100 or that the behaviour if it does is undefined.  It would take someone
> > to poke the registers on a real device to find out.  Either way, this seems to
> > be a reasonable hardening against corrupt data back from the device.
> > 
> > I will leave it on list a while longer though so it is probably 6.2 material
> > now.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > > ---
> > >  drivers/iio/filter/admv8818.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/filter/admv8818.c b/drivers/iio/filter/admv8818.c
> > > index 68de45fe21b4..fe8d46cb7f1d 100644
> > > --- a/drivers/iio/filter/admv8818.c
> > > +++ b/drivers/iio/filter/admv8818.c
> > > @@ -265,7 +265,7 @@ static int __admv8818_read_hpf_freq(struct  
> > admv8818_state *st, u64 *hpf_freq)  
> > >  		return ret;
> > >
> > >  	hpf_band = FIELD_GET(ADMV8818_SW_IN_WR0_MSK, data);
> > > -	if (!hpf_band) {
> > > +	if (!hpf_band || hpf_band > 4) {
> > >  		*hpf_freq = 0;
> > >  		return ret;
> > >  	}
> > > @@ -303,7 +303,7 @@ static int __admv8818_read_lpf_freq(struct  
> > admv8818_state *st, u64 *lpf_freq)  
> > >  		return ret;
> > >
> > >  	lpf_band = FIELD_GET(ADMV8818_SW_OUT_WR0_MSK, data);
> > > -	if (!lpf_band) {
> > > +	if (!lpf_band || lpf_band > 4) {
> > >  		*lpf_freq = 0;
> > >  		return ret;
> > >  	}  
>
diff mbox series

Patch

diff --git a/drivers/iio/filter/admv8818.c b/drivers/iio/filter/admv8818.c
index 68de45fe21b4..fe8d46cb7f1d 100644
--- a/drivers/iio/filter/admv8818.c
+++ b/drivers/iio/filter/admv8818.c
@@ -265,7 +265,7 @@  static int __admv8818_read_hpf_freq(struct admv8818_state *st, u64 *hpf_freq)
 		return ret;
 
 	hpf_band = FIELD_GET(ADMV8818_SW_IN_WR0_MSK, data);
-	if (!hpf_band) {
+	if (!hpf_band || hpf_band > 4) {
 		*hpf_freq = 0;
 		return ret;
 	}
@@ -303,7 +303,7 @@  static int __admv8818_read_lpf_freq(struct admv8818_state *st, u64 *lpf_freq)
 		return ret;
 
 	lpf_band = FIELD_GET(ADMV8818_SW_OUT_WR0_MSK, data);
-	if (!lpf_band) {
+	if (!lpf_band || lpf_band > 4) {
 		*lpf_freq = 0;
 		return ret;
 	}