drivers/iio/adc/qcom-pm8xxx-xoadc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
The left side of the loop condition never becomes false.
hwchan cannot be NULL, because it points to elements of the
hw_channels array that takes one of 4 predefined values:
pm8018_xoadc_channels, pm8038_xoadc_channels,
pm8058_xoadc_channels, pm8921_xoadc_channels.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC")
Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru>
---
drivers/iio/adc/qcom-pm8xxx-xoadc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
index eb424496ee1d..64a3aeb6261c 100644
--- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
+++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
@@ -758,7 +758,7 @@ static int pm8xxx_xoadc_parse_channel(struct device *dev,
/* Find the right channel setting */
chid = 0;
hwchan = &hw_channels[0];
- while (hwchan && hwchan->datasheet_name) {
+ while (hwchan->datasheet_name) {
if (hwchan->pre_scale_mux == pre_scale_mux &&
hwchan->amux_channel == amux_channel)
break;
--
2.34.1On Tue, Mar 14, 2023 at 8:37 PM Kasumov Ruslan <xhxgldhlpfy@gmail.com> wrote:
> The left side of the loop condition never becomes false.
> hwchan cannot be NULL, because it points to elements of the
> hw_channels array that takes one of 4 predefined values:
> pm8018_xoadc_channels, pm8038_xoadc_channels,
> pm8058_xoadc_channels, pm8921_xoadc_channels.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC")
> Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
On Tue, Mar 14, 2023 at 8:37 PM Kasumov Ruslan <xhxgldhlpfy@gmail.com> wrote:
> The left side of the loop condition never becomes false.
> hwchan cannot be NULL, because it points to elements of the
> hw_channels array that takes one of 4 predefined values:
> pm8018_xoadc_channels, pm8038_xoadc_channels,
> pm8058_xoadc_channels, pm8921_xoadc_channels.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
I am not impressed with that tool. See below:
> Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC")
> Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru>
(...)
> hwchan = &hw_channels[0];
> - while (hwchan && hwchan->datasheet_name) {
> + while (hwchan->datasheet_name) {
> if (hwchan->pre_scale_mux == pre_scale_mux &&
> hwchan->amux_channel == amux_channel)
> break;
NAK have you tested this on a real system?
Here is the complete loop:
hwchan = &hw_channels[0];
while (hwchan && hwchan->datasheet_name) {
if (hwchan->pre_scale_mux == pre_scale_mux &&
hwchan->amux_channel == amux_channel)
break;
hwchan++;
chid++;
}
Notice how hwchan is used as iterator in hwchan++.
What you are doing will cause a zero-pointer dereference.
Whoever is developing "SVACE" needs to fix the tool to understand
this kind of usage.
Yours,
Linus Walleij
On Tue, Mar 14, 2023 at 10:12 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Mar 14, 2023 at 8:37 PM Kasumov Ruslan <xhxgldhlpfy@gmail.com> wrote:
>
> > The left side of the loop condition never becomes false.
> > hwchan cannot be NULL, because it points to elements of the
> > hw_channels array that takes one of 4 predefined values:
> > pm8018_xoadc_channels, pm8038_xoadc_channels,
> > pm8058_xoadc_channels, pm8921_xoadc_channels.
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> I am not impressed with that tool. See below:
>
> > Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC")
> > Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru>
>
> (...)
> > hwchan = &hw_channels[0];
> > - while (hwchan && hwchan->datasheet_name) {
> > + while (hwchan->datasheet_name) {
> > if (hwchan->pre_scale_mux == pre_scale_mux &&
> > hwchan->amux_channel == amux_channel)
> > break;
>
> NAK have you tested this on a real system?
>
> Here is the complete loop:
>
> hwchan = &hw_channels[0];
> while (hwchan && hwchan->datasheet_name) {
> if (hwchan->pre_scale_mux == pre_scale_mux &&
> hwchan->amux_channel == amux_channel)
> break;
> hwchan++;
> chid++;
> }
>
> Notice how hwchan is used as iterator in hwchan++.
>
> What you are doing will cause a zero-pointer dereference.
Nah the AI is smarter than me this time, I'm wrong, I think :(
hwchan is indeed never NULL here, and the code immediately
after unconditionally dereferences hwchan->datasheet_name.
Who wrote this convoluted code again:
63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 759)
chid = 0;
63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 760)
hwchan = &hw_channels[0];
63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 761)
while (hwchan && hwchan->datasheet_name) {
63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 762)
if (hwchan->pre_scale_mux == pre_scale_mux &&
63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 763)
hwchan->amux_channel == amux_channel)
63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 764)
break;
63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 765)
hwchan++;
63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 766)
chid++;
63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 767) }
63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 768)
/* The sentinel does not have a name assigned */
63c3ecd946d4a (Linus Walleij 2017-04-04 14:08:19 +0200 769)
if (!hwchan->datasheet_name) {
Oh that guy ...
I wonder if we can make it look better and less unintuitive.
Yours,
Linus Walleij
Hi,
> > The left side of the loop condition never becomes false.
> > hwchan cannot be NULL, because it points to elements of the
> > hw_channels array that takes one of 4 predefined values:
> > pm8018_xoadc_channels, pm8038_xoadc_channels,
> > pm8058_xoadc_channels, pm8921_xoadc_channels.
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> I am not impressed with that tool. See below:
>
> > Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC")
> > Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru>
>
> (...)
> > hwchan = &hw_channels[0];
> > - while (hwchan && hwchan->datasheet_name) {
> > + while (hwchan->datasheet_name) {
> > if (hwchan->pre_scale_mux == pre_scale_mux &&
> > hwchan->amux_channel == amux_channel)
> > break;
>
> NAK have you tested this on a real system?
>
> Here is the complete loop:
>
> hwchan = &hw_channels[0];
> while (hwchan && hwchan->datasheet_name) {
> if (hwchan->pre_scale_mux == pre_scale_mux &&
> hwchan->amux_channel == amux_channel)
> break;
> hwchan++;
ops, yes, sorry, I overlooked at this... This becomes NULL at
some point as there is a sentinel in the _variant structures.
Thanks,
Andi
> chid++;
> }
>
> Notice how hwchan is used as iterator in hwchan++.
>
> What you are doing will cause a zero-pointer dereference.
>
> Whoever is developing "SVACE" needs to fix the tool to understand
> this kind of usage.
>
> Yours,
> Linus Walleij
On 15.03.2023 00:28, Andi Shyti wrote:
> Hi,
>
>>> The left side of the loop condition never becomes false.
>>> hwchan cannot be NULL, because it points to elements of the
>>> hw_channels array that takes one of 4 predefined values:
>>> pm8018_xoadc_channels, pm8038_xoadc_channels,
>>> pm8058_xoadc_channels, pm8921_xoadc_channels.
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> I am not impressed with that tool. See below:
>>
>>> Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC")
>>> Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru>
>>
>> (...)
>>> hwchan = &hw_channels[0];
>>> - while (hwchan && hwchan->datasheet_name) {
>>> + while (hwchan->datasheet_name) {
>>> if (hwchan->pre_scale_mux == pre_scale_mux &&
>>> hwchan->amux_channel == amux_channel)
>>> break;
>>
>> NAK have you tested this on a real system?
>>
>> Here is the complete loop:
>>
>> hwchan = &hw_channels[0];
>> while (hwchan && hwchan->datasheet_name) {
>> if (hwchan->pre_scale_mux == pre_scale_mux &&
>> hwchan->amux_channel == amux_channel)
>> break;
>> hwchan++;
>
> ops, yes, sorry, I overlooked at this... This becomes NULL at
> some point as there is a sentinel in the _variant structures.
>
Could you please clarify what do you mean.
As far as I can see sentinel is an "empty" element of xoadc_channel in
the array, i.e. hwchan->datasheet_name works as a sentinel while hwchan
is always non NULL.
--
Alexey
On Tue, Mar 14, 2023 at 11:03 PM Alexey Khoroshilov <khoroshilov@ispras.ru> wrote: > As far as I can see sentinel is an "empty" element of xoadc_channel in > the array, i.e. hwchan->datasheet_name works as a sentinel while hwchan > is always non NULL. You're right, I was unable to understand my own code :( Yours, Linus Walleij
Hi Alexey and Ruslan, On Tue, Mar 14, 2023 at 11:07:19PM +0100, Linus Walleij wrote: > On Tue, Mar 14, 2023 at 11:03 PM Alexey Khoroshilov > <khoroshilov@ispras.ru> wrote: > > > As far as I can see sentinel is an "empty" element of xoadc_channel in > > the array, i.e. hwchan->datasheet_name works as a sentinel while hwchan > > is always non NULL. > > You're right, I was unable to understand my own code :( At this time of the day I got alarmed too. Happens :) Please ignore my previous comment but still no need for the Fixes: tag from the commit log as it's a cleanup and not a bug fix. Thanks, Andi
On Tue, Mar 14, 2023 at 10:37:09PM +0300, Kasumov Ruslan wrote:
> The left side of the loop condition never becomes false.
> hwchan cannot be NULL, because it points to elements of the
> hw_channels array that takes one of 4 predefined values:
> pm8018_xoadc_channels, pm8038_xoadc_channels,
> pm8058_xoadc_channels, pm8921_xoadc_channels.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC")
It wasn't broken before, it wasn't causing any harm. It's not a
fix, it's a cleanup. Please, remove remove the "Fixes:" tag.
> Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru>
> ---
> drivers/iio/adc/qcom-pm8xxx-xoadc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> index eb424496ee1d..64a3aeb6261c 100644
> --- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> +++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> @@ -758,7 +758,7 @@ static int pm8xxx_xoadc_parse_channel(struct device *dev,
> /* Find the right channel setting */
> chid = 0;
> hwchan = &hw_channels[0];
> - while (hwchan && hwchan->datasheet_name) {
> + while (hwchan->datasheet_name) {
With the fixes tag removed:
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Thanks,
Andi
> if (hwchan->pre_scale_mux == pre_scale_mux &&
> hwchan->amux_channel == amux_channel)
> break;
> --
> 2.34.1
>
The left side of the loop condition never becomes false.
hwchan cannot be NULL, because it points to elements of the
hw_channels array that takes one of 4 predefined values:
pm8018_xoadc_channels, pm8038_xoadc_channels,
pm8058_xoadc_channels, pm8921_xoadc_channels.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru>
---
v2: Removed "Fixes" tag as Andi Shyti <andi.shyti@kernel.org> suggested.
drivers/iio/adc/qcom-pm8xxx-xoadc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
index eb424496ee1d..64a3aeb6261c 100644
--- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
+++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
@@ -758,7 +758,7 @@ static int pm8xxx_xoadc_parse_channel(struct device *dev,
/* Find the right channel setting */
chid = 0;
hwchan = &hw_channels[0];
- while (hwchan && hwchan->datasheet_name) {
+ while (hwchan->datasheet_name) {
if (hwchan->pre_scale_mux == pre_scale_mux &&
hwchan->amux_channel == amux_channel)
break;
--
2.34.1On Wed, 15 Mar 2023 16:51:14 +0300
Kasumov Ruslan <xhxgldhlpfy@gmail.com> wrote:
> The left side of the loop condition never becomes false.
> hwchan cannot be NULL, because it points to elements of the
> hw_channels array that takes one of 4 predefined values:
> pm8018_xoadc_channels, pm8038_xoadc_channels,
> pm8058_xoadc_channels, pm8921_xoadc_channels.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Kasumov Ruslan <s02210418@gse.cs.msu.ru>
We could file this under the category of (overly) paranoid checking that
he variant structure has the hw_channels set, but given there are only 4 instances
of that it is (as you have done) easy to check.
So fair enough to drop this check.
Applied.
Thanks,
Jonathan
> ---
> v2: Removed "Fixes" tag as Andi Shyti <andi.shyti@kernel.org> suggested.
> drivers/iio/adc/qcom-pm8xxx-xoadc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> index eb424496ee1d..64a3aeb6261c 100644
> --- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> +++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> @@ -758,7 +758,7 @@ static int pm8xxx_xoadc_parse_channel(struct device *dev,
> /* Find the right channel setting */
> chid = 0;
> hwchan = &hw_channels[0];
> - while (hwchan && hwchan->datasheet_name) {
> + while (hwchan->datasheet_name) {
> if (hwchan->pre_scale_mux == pre_scale_mux &&
> hwchan->amux_channel == amux_channel)
> break;
© 2016 - 2026 Red Hat, Inc.