* [PATCH] iio: vadc: Fix potential dereference of NULL pointer @ 2022-05-18 6:43 Yongzhi Liu 2022-05-18 17:31 ` Matthias Kaehlcke 0 siblings, 1 reply; 13+ messages in thread From: Yongzhi Liu @ 2022-05-18 6:43 UTC (permalink / raw) To: agross, bjorn.andersson, jic23, lars Cc: linux-arm-msm, linux-iio, linux-kernel, fuyq, Yongzhi Liu The return value of vadc_get_channel() needs to be checked to avoid use of NULL pointer, which is followed by the caller 'vadc_do_conversion' of function 'vadc_configure'. Fix this by adding the null pointer check on prop in function 'vadc_configure'. Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> --- drivers/iio/adc/qcom-spmi-vadc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c index 34202ba..d99bd72 100644 --- a/drivers/iio/adc/qcom-spmi-vadc.c +++ b/drivers/iio/adc/qcom-spmi-vadc.c @@ -210,6 +210,9 @@ static int vadc_configure(struct vadc_priv *vadc, u8 decimation, mode_ctrl; int ret; + if (!prop) + return -ENODEV; + /* Mode selection */ mode_ctrl = (VADC_OP_MODE_NORMAL << VADC_OP_MODE_SHIFT) | VADC_ADC_TRIM_EN | VADC_AMUX_TRIM_EN; -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] iio: vadc: Fix potential dereference of NULL pointer 2022-05-18 6:43 [PATCH] iio: vadc: Fix potential dereference of NULL pointer Yongzhi Liu @ 2022-05-18 17:31 ` Matthias Kaehlcke 2022-05-19 5:50 ` [PATCH v2] " Yongzhi Liu 0 siblings, 1 reply; 13+ messages in thread From: Matthias Kaehlcke @ 2022-05-18 17:31 UTC (permalink / raw) To: Yongzhi Liu Cc: agross, bjorn.andersson, jic23, lars, linux-arm-msm, linux-iio, linux-kernel, fuyq On Tue, May 17, 2022 at 11:43:00PM -0700, Yongzhi Liu wrote: > The return value of vadc_get_channel() needs to be checked > to avoid use of NULL pointer, which is followed by > the caller 'vadc_do_conversion' of function 'vadc_configure'. > Fix this by adding the null pointer check on prop > in function 'vadc_configure'. > > Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> > --- > drivers/iio/adc/qcom-spmi-vadc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c > index 34202ba..d99bd72 100644 > --- a/drivers/iio/adc/qcom-spmi-vadc.c > +++ b/drivers/iio/adc/qcom-spmi-vadc.c > @@ -210,6 +210,9 @@ static int vadc_configure(struct vadc_priv *vadc, > u8 decimation, mode_ctrl; > int ret; > > + if (!prop) > + return -ENODEV; > + > /* Mode selection */ > mode_ctrl = (VADC_OP_MODE_NORMAL << VADC_OP_MODE_SHIFT) | > VADC_ADC_TRIM_EN | VADC_AMUX_TRIM_EN; Shouldn't the check be done in vadc_measure_ref_points() where 'prop' is obtained, rather than deep down in the call chain? For example vadc_do_conversion() would also dereference the NULL pointer unless one of the prior function calls fails. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] iio: vadc: Fix potential dereference of NULL pointer 2022-05-18 17:31 ` Matthias Kaehlcke @ 2022-05-19 5:50 ` Yongzhi Liu 2022-05-20 17:13 ` Jonathan Cameron 0 siblings, 1 reply; 13+ messages in thread From: Yongzhi Liu @ 2022-05-19 5:50 UTC (permalink / raw) To: agross, bjorn.andersson, jic23, lars Cc: linux-arm-msm, linux-iio, linux-kernel, fuyq, Yongzhi Liu The return value of vadc_get_channel() needs to be checked to avoid use of NULL pointer. Fix this by adding the null pointer check on prop. Fixes: 0917de94c ("iio: vadc: Qualcomm SPMI PMIC voltage ADC driver") Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> --- drivers/iio/adc/qcom-spmi-vadc.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c index 34202ba..9fa61fb 100644 --- a/drivers/iio/adc/qcom-spmi-vadc.c +++ b/drivers/iio/adc/qcom-spmi-vadc.c @@ -358,14 +358,25 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) vadc->graph[VADC_CALIB_ABSOLUTE].dx = VADC_ABSOLUTE_RANGE_UV; prop = vadc_get_channel(vadc, VADC_REF_1250MV); + if (!prop) { + dev_err(vadc->dev, "Please define 1.25V channel\n"); + ret = -ENODEV; + goto err; + } ret = vadc_do_conversion(vadc, prop, &read_1); if (ret) goto err; /* Try with buffered 625mV channel first */ prop = vadc_get_channel(vadc, VADC_SPARE1); - if (!prop) + if (!prop) { prop = vadc_get_channel(vadc, VADC_REF_625MV); + if (!prop) { + dev_err(vadc->dev, "Please define 0.625V channel\n"); + ret = -ENODEV; + goto err; + } + } ret = vadc_do_conversion(vadc, prop, &read_2); if (ret) @@ -381,11 +392,21 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) /* Ratiometric calibration */ prop = vadc_get_channel(vadc, VADC_VDD_VADC); + if (!prop) { + dev_err(vadc->dev, "Please define VDD channel\n"); + ret = -ENODEV; + goto err; + } ret = vadc_do_conversion(vadc, prop, &read_1); if (ret) goto err; prop = vadc_get_channel(vadc, VADC_GND_REF); + if (!prop) { + dev_err(vadc->dev, "Please define GND channel\n"); + ret = -ENODEV; + goto err; + } ret = vadc_do_conversion(vadc, prop, &read_2); if (ret) goto err; -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] iio: vadc: Fix potential dereference of NULL pointer 2022-05-19 5:50 ` [PATCH v2] " Yongzhi Liu @ 2022-05-20 17:13 ` Jonathan Cameron 2022-05-21 3:31 ` [PATCH] hv_netvsc: " Yongzhi Liu 2022-05-21 3:35 ` [PATCH v3] iio: vadc: " Yongzhi Liu 0 siblings, 2 replies; 13+ messages in thread From: Jonathan Cameron @ 2022-05-20 17:13 UTC (permalink / raw) To: Yongzhi Liu Cc: agross, bjorn.andersson, jic23, lars, linux-arm-msm, linux-iio, linux-kernel, fuyq On Wed, 18 May 2022 22:50:55 -0700 Yongzhi Liu <lyz_cs@pku.edu.cn> wrote: > The return value of vadc_get_channel() needs to be checked > to avoid use of NULL pointer. Fix this by adding the null > pointer check on prop. > > Fixes: 0917de94c ("iio: vadc: Qualcomm SPMI PMIC voltage ADC driver") > > Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> This function has a lot of goto err; where err just results in a print. My suggestion is to just drop that print and use error specific prints as you have done here, then use direct returns. > --- > drivers/iio/adc/qcom-spmi-vadc.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c > index 34202ba..9fa61fb 100644 > --- a/drivers/iio/adc/qcom-spmi-vadc.c > +++ b/drivers/iio/adc/qcom-spmi-vadc.c > @@ -358,14 +358,25 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) > vadc->graph[VADC_CALIB_ABSOLUTE].dx = VADC_ABSOLUTE_RANGE_UV; > > prop = vadc_get_channel(vadc, VADC_REF_1250MV); > + if (!prop) { > + dev_err(vadc->dev, "Please define 1.25V channel\n"); > + ret = -ENODEV; > + goto err; > + } > ret = vadc_do_conversion(vadc, prop, &read_1); > if (ret) > goto err; > > /* Try with buffered 625mV channel first */ > prop = vadc_get_channel(vadc, VADC_SPARE1); > - if (!prop) > + if (!prop) { > prop = vadc_get_channel(vadc, VADC_REF_625MV); > + if (!prop) { > + dev_err(vadc->dev, "Please define 0.625V channel\n"); > + ret = -ENODEV; > + goto err; > + } > + } > > ret = vadc_do_conversion(vadc, prop, &read_2); > if (ret) > @@ -381,11 +392,21 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) > > /* Ratiometric calibration */ > prop = vadc_get_channel(vadc, VADC_VDD_VADC); > + if (!prop) { > + dev_err(vadc->dev, "Please define VDD channel\n"); > + ret = -ENODEV; > + goto err; > + } > ret = vadc_do_conversion(vadc, prop, &read_1); > if (ret) > goto err; > > prop = vadc_get_channel(vadc, VADC_GND_REF); > + if (!prop) { > + dev_err(vadc->dev, "Please define GND channel\n"); > + ret = -ENODEV; > + goto err; > + } > ret = vadc_do_conversion(vadc, prop, &read_2); > if (ret) > goto err; ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] hv_netvsc: Fix potential dereference of NULL pointer 2022-05-20 17:13 ` Jonathan Cameron @ 2022-05-21 3:31 ` Yongzhi Liu 2022-05-21 3:34 ` 刘永志 2022-05-23 15:21 ` Andy Shevchenko 2022-05-21 3:35 ` [PATCH v3] iio: vadc: " Yongzhi Liu 1 sibling, 2 replies; 13+ messages in thread From: Yongzhi Liu @ 2022-05-21 3:31 UTC (permalink / raw) To: agross, bjorn.andersson, jic23, lars, svarbanov, iivanov, jonathan.cameron Cc: linux-arm-msm, linux-iio, linux-kernel, fuyq, Yongzhi Liu The return value of netvsc_devinfo_get() needs to be checked to avoid use of NULL pointer in case of an allocation failure. Fixes: 0efeea5fb ("hv_netvsc: Add the support of hibernation") Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> --- drivers/net/hyperv/netvsc_drv.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index fde1c49..b1dece6 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -2671,7 +2671,10 @@ static int netvsc_suspend(struct hv_device *dev) /* Save the current config info */ ndev_ctx->saved_netvsc_dev_info = netvsc_devinfo_get(nvdev); - + if (!ndev_ctx->saved_netvsc_dev_info) { + ret = -ENOMEM; + goto out; + } ret = netvsc_detach(net, nvdev); out: rtnl_unlock(); -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] hv_netvsc: Fix potential dereference of NULL pointer 2022-05-21 3:31 ` [PATCH] hv_netvsc: " Yongzhi Liu @ 2022-05-21 3:34 ` 刘永志 2022-05-23 15:21 ` Andy Shevchenko 1 sibling, 0 replies; 13+ messages in thread From: 刘永志 @ 2022-05-21 3:34 UTC (permalink / raw) To: agross, bjorn.andersson, jic23, lars, svarbanov, iivanov, jonathan.cameron Cc: linux-arm-msm, linux-iio, linux-kernel I'm sorry to send this to linux-iio by mistake. I will cautiously submit patches later. > -----Original Messages----- > From: "Yongzhi Liu" <lyz_cs@pku.edu.cn> > Sent Time: 2022-05-21 11:31:02 (Saturday) > To: agross@kernel.org, bjorn.andersson@linaro.org, jic23@kernel.org, lars@metafoo.de, svarbanov@mm-sol.com, iivanov@mm-sol.com, jonathan.cameron@huawei.com > Cc: linux-arm-msm@vger.kernel.org, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, fuyq@stu.pku.edu.cn, "Yongzhi Liu" <lyz_cs@pku.edu.cn> > Subject: [PATCH] hv_netvsc: Fix potential dereference of NULL pointer > > The return value of netvsc_devinfo_get() > needs to be checked to avoid use of NULL > pointer in case of an allocation failure. > > Fixes: 0efeea5fb ("hv_netvsc: Add the support of hibernation") > > Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> > --- > drivers/net/hyperv/netvsc_drv.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index fde1c49..b1dece6 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -2671,7 +2671,10 @@ static int netvsc_suspend(struct hv_device *dev) > > /* Save the current config info */ > ndev_ctx->saved_netvsc_dev_info = netvsc_devinfo_get(nvdev); > - > + if (!ndev_ctx->saved_netvsc_dev_info) { > + ret = -ENOMEM; > + goto out; > + } > ret = netvsc_detach(net, nvdev); > out: > rtnl_unlock(); > -- > 2.7.4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] hv_netvsc: Fix potential dereference of NULL pointer 2022-05-21 3:31 ` [PATCH] hv_netvsc: " Yongzhi Liu 2022-05-21 3:34 ` 刘永志 @ 2022-05-23 15:21 ` Andy Shevchenko 2022-05-23 15:45 ` 刘永志 1 sibling, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2022-05-23 15:21 UTC (permalink / raw) To: Yongzhi Liu Cc: Andy Gross, Bjorn Andersson, Jonathan Cameron, Lars-Peter Clausen, Stanimir Varbanov, Ivan T. Ivanov, Jonathan Cameron, linux-arm-msm, linux-iio, Linux Kernel Mailing List, fuyq On Sat, May 21, 2022 at 6:27 AM Yongzhi Liu <lyz_cs@pku.edu.cn> wrote: > > The return value of netvsc_devinfo_get() > needs to be checked to avoid use of NULL > pointer in case of an allocation failure. > Fixes: 0efeea5fb ("hv_netvsc: Add the support of hibernation") > > Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> For the future independently of the subsystem or mailing list, the tag block (above) mustn't have lank lines. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: [PATCH] hv_netvsc: Fix potential dereference of NULL pointer 2022-05-23 15:21 ` Andy Shevchenko @ 2022-05-23 15:45 ` 刘永志 0 siblings, 0 replies; 13+ messages in thread From: 刘永志 @ 2022-05-23 15:45 UTC (permalink / raw) To: andy shevchenko Cc: andy gross, bjorn andersson, jonathan cameron, lars-peter clausen, stanimir varbanov, ivan t. ivanov, jonathan cameron, linux-arm-msm, linux-iio, linux kernel mailing list, fuyq > -----Original Messages----- > From: "Andy Shevchenko" <andy.shevchenko@gmail.com> > Sent Time: 2022-05-23 23:21:54 (Monday) > To: "Yongzhi Liu" <lyz_cs@pku.edu.cn> > Cc: "Andy Gross" <agross@kernel.org>, "Bjorn Andersson" <bjorn.andersson@linaro.org>, "Jonathan Cameron" <jic23@kernel.org>, "Lars-Peter Clausen" <lars@metafoo.de>, "Stanimir Varbanov" <svarbanov@mm-sol.com>, "Ivan T. Ivanov" <iivanov@mm-sol.com>, "Jonathan Cameron" <jonathan.cameron@huawei.com>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, linux-iio <linux-iio@vger.kernel.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, fuyq@stu.pku.edu.cn > Subject: Re: [PATCH] hv_netvsc: Fix potential dereference of NULL pointer > > On Sat, May 21, 2022 at 6:27 AM Yongzhi Liu <lyz_cs@pku.edu.cn> wrote: > > > > The return value of netvsc_devinfo_get() > > needs to be checked to avoid use of NULL > > pointer in case of an allocation failure. > > > Fixes: 0efeea5fb ("hv_netvsc: Add the support of hibernation") > > > > Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> > Thanks for your reply and advice. > For the future independently of the subsystem or mailing list, the tag > block (above) mustn't have lank lines. > > -- > With Best Regards, > Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] iio: vadc: Fix potential dereference of NULL pointer 2022-05-20 17:13 ` Jonathan Cameron 2022-05-21 3:31 ` [PATCH] hv_netvsc: " Yongzhi Liu @ 2022-05-21 3:35 ` Yongzhi Liu 2022-05-22 11:01 ` Jonathan Cameron 1 sibling, 1 reply; 13+ messages in thread From: Yongzhi Liu @ 2022-05-21 3:35 UTC (permalink / raw) To: agross, bjorn.andersson, jic23, lars, svarbanov, iivanov, jonathan.cameron Cc: linux-arm-msm, linux-iio, linux-kernel, fuyq, Yongzhi Liu The return value of vadc_get_channel() needs to be checked to avoid use of NULL pointer. Fix this by adding the null pointer check on prop and dropping general error prints. Fixes: 0917de94c02f ("iio: vadc: Qualcomm SPMI PMIC voltage ADC driver") Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> --- drivers/iio/adc/qcom-spmi-vadc.c | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c index 34202ba..43a52b1 100644 --- a/drivers/iio/adc/qcom-spmi-vadc.c +++ b/drivers/iio/adc/qcom-spmi-vadc.c @@ -358,22 +358,33 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) vadc->graph[VADC_CALIB_ABSOLUTE].dx = VADC_ABSOLUTE_RANGE_UV; prop = vadc_get_channel(vadc, VADC_REF_1250MV); + if (!prop) { + dev_err(vadc->dev, "Please define 1.25V channel\n"); + ret = -ENODEV; + return ret; + } ret = vadc_do_conversion(vadc, prop, &read_1); if (ret) - goto err; + return ret; /* Try with buffered 625mV channel first */ prop = vadc_get_channel(vadc, VADC_SPARE1); - if (!prop) + if (!prop) { prop = vadc_get_channel(vadc, VADC_REF_625MV); + if (!prop) { + dev_err(vadc->dev, "Please define 0.625V channel\n"); + ret = -ENODEV; + return ret; + } + } ret = vadc_do_conversion(vadc, prop, &read_2); if (ret) - goto err; + return ret; if (read_1 == read_2) { ret = -EINVAL; - goto err; + return ret; } vadc->graph[VADC_CALIB_ABSOLUTE].dy = read_1 - read_2; @@ -381,25 +392,32 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) /* Ratiometric calibration */ prop = vadc_get_channel(vadc, VADC_VDD_VADC); + if (!prop) { + dev_err(vadc->dev, "Please define VDD channel\n"); + ret = -ENODEV; + return ret; + } ret = vadc_do_conversion(vadc, prop, &read_1); if (ret) - goto err; + return ret; prop = vadc_get_channel(vadc, VADC_GND_REF); + if (!prop) { + dev_err(vadc->dev, "Please define GND channel\n"); + ret = -ENODEV; + return ret; + } ret = vadc_do_conversion(vadc, prop, &read_2); if (ret) - goto err; + return ret; if (read_1 == read_2) { ret = -EINVAL; - goto err; + return ret; } vadc->graph[VADC_CALIB_RATIOMETRIC].dy = read_1 - read_2; vadc->graph[VADC_CALIB_RATIOMETRIC].gnd = read_2; -err: - if (ret) - dev_err(vadc->dev, "measure reference points failed\n"); return ret; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3] iio: vadc: Fix potential dereference of NULL pointer 2022-05-21 3:35 ` [PATCH v3] iio: vadc: " Yongzhi Liu @ 2022-05-22 11:01 ` Jonathan Cameron 2022-05-22 16:53 ` [PATCH v4] " Yongzhi Liu 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Cameron @ 2022-05-22 11:01 UTC (permalink / raw) To: Yongzhi Liu Cc: agross, bjorn.andersson, lars, svarbanov, iivanov, jonathan.cameron, linux-arm-msm, linux-iio, linux-kernel, fuyq On Fri, 20 May 2022 20:35:35 -0700 Yongzhi Liu <lyz_cs@pku.edu.cn> wrote: > The return value of vadc_get_channel() needs to be checked to > avoid use of NULL pointer. Fix this by adding the null pointer > check on prop and dropping general error prints. > > Fixes: 0917de94c02f ("iio: vadc: Qualcomm SPMI PMIC voltage ADC driver") > Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> Hi, Heading in the right direction. A few comments inline. Thanks, Jonathan > --- > drivers/iio/adc/qcom-spmi-vadc.c | 38 ++++++++++++++++++++++++++++---------- > 1 file changed, 28 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c > index 34202ba..43a52b1 100644 > --- a/drivers/iio/adc/qcom-spmi-vadc.c > +++ b/drivers/iio/adc/qcom-spmi-vadc.c > @@ -358,22 +358,33 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) > vadc->graph[VADC_CALIB_ABSOLUTE].dx = VADC_ABSOLUTE_RANGE_UV; > > prop = vadc_get_channel(vadc, VADC_REF_1250MV); > + if (!prop) { > + dev_err(vadc->dev, "Please define 1.25V channel\n"); > + ret = -ENODEV; > + return ret; return -ENODEV; Same for the other case below. > + } > ret = vadc_do_conversion(vadc, prop, &read_1); > if (ret) > - goto err; Good to add a note to the patch description that vadc_do_conversion() already provides error prints in at least some of it's error paths. Thus it is reasonable to drop the extra reporting in this function. > + return ret; > > /* Try with buffered 625mV channel first */ > prop = vadc_get_channel(vadc, VADC_SPARE1); > - if (!prop) > + if (!prop) { > prop = vadc_get_channel(vadc, VADC_REF_625MV); > + if (!prop) { > + dev_err(vadc->dev, "Please define 0.625V channel\n"); > + ret = -ENODEV; > + return ret; > + } > + } > > ret = vadc_do_conversion(vadc, prop, &read_2); > if (ret) > - goto err; > + return ret; > > if (read_1 == read_2) { > ret = -EINVAL; > - goto err; > + return ret; > } > > vadc->graph[VADC_CALIB_ABSOLUTE].dy = read_1 - read_2; > @@ -381,25 +392,32 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) > > /* Ratiometric calibration */ > prop = vadc_get_channel(vadc, VADC_VDD_VADC); > + if (!prop) { > + dev_err(vadc->dev, "Please define VDD channel\n"); > + ret = -ENODEV; > + return ret; > + } > ret = vadc_do_conversion(vadc, prop, &read_1); > if (ret) > - goto err; > + return ret; > > prop = vadc_get_channel(vadc, VADC_GND_REF); > + if (!prop) { > + dev_err(vadc->dev, "Please define GND channel\n"); > + ret = -ENODEV; > + return ret; > + } > ret = vadc_do_conversion(vadc, prop, &read_2); > if (ret) > - goto err; > + return ret; > > if (read_1 == read_2) { > ret = -EINVAL; > - goto err; > + return ret; > } > > vadc->graph[VADC_CALIB_RATIOMETRIC].dy = read_1 - read_2; > vadc->graph[VADC_CALIB_RATIOMETRIC].gnd = read_2; > -err: > - if (ret) > - dev_err(vadc->dev, "measure reference points failed\n"); > > return ret; > } ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4] iio: vadc: Fix potential dereference of NULL pointer 2022-05-22 11:01 ` Jonathan Cameron @ 2022-05-22 16:53 ` Yongzhi Liu 2022-06-03 15:02 ` Jonathan Cameron 0 siblings, 1 reply; 13+ messages in thread From: Yongzhi Liu @ 2022-05-22 16:53 UTC (permalink / raw) To: jic23, agross, bjorn.andersson, lars, svarbanov, iivanov, jonathan.cameron Cc: linux-arm-msm, linux-iio, linux-kernel, fuyq, Yongzhi Liu The return value of vadc_get_channel() needs to be checked to avoid use of NULL pointer. vadc_do_conversion() already provides error prints in at least some of it's error paths. Thus it is reasonable to add the null pointer check on prop and drop the extra reporting in vadc_measure_ref_points(). Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> --- drivers/iio/adc/qcom-spmi-vadc.c | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c index 34202ba..43a52b1 100644 --- a/drivers/iio/adc/qcom-spmi-vadc.c +++ b/drivers/iio/adc/qcom-spmi-vadc.c @@ -358,22 +358,33 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) vadc->graph[VADC_CALIB_ABSOLUTE].dx = VADC_ABSOLUTE_RANGE_UV; prop = vadc_get_channel(vadc, VADC_REF_1250MV); + if (!prop) { + dev_err(vadc->dev, "Please define 1.25V channel\n"); + ret = -ENODEV; + return ret; + } ret = vadc_do_conversion(vadc, prop, &read_1); if (ret) - goto err; + return ret; /* Try with buffered 625mV channel first */ prop = vadc_get_channel(vadc, VADC_SPARE1); - if (!prop) + if (!prop) { prop = vadc_get_channel(vadc, VADC_REF_625MV); + if (!prop) { + dev_err(vadc->dev, "Please define 0.625V channel\n"); + ret = -ENODEV; + return ret; + } + } ret = vadc_do_conversion(vadc, prop, &read_2); if (ret) - goto err; + return ret; if (read_1 == read_2) { ret = -EINVAL; - goto err; + return ret; } vadc->graph[VADC_CALIB_ABSOLUTE].dy = read_1 - read_2; @@ -381,25 +392,32 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) /* Ratiometric calibration */ prop = vadc_get_channel(vadc, VADC_VDD_VADC); + if (!prop) { + dev_err(vadc->dev, "Please define VDD channel\n"); + ret = -ENODEV; + return ret; + } ret = vadc_do_conversion(vadc, prop, &read_1); if (ret) - goto err; + return ret; prop = vadc_get_channel(vadc, VADC_GND_REF); + if (!prop) { + dev_err(vadc->dev, "Please define GND channel\n"); + ret = -ENODEV; + return ret; + } ret = vadc_do_conversion(vadc, prop, &read_2); if (ret) - goto err; + return ret; if (read_1 == read_2) { ret = -EINVAL; - goto err; + return ret; } vadc->graph[VADC_CALIB_RATIOMETRIC].dy = read_1 - read_2; vadc->graph[VADC_CALIB_RATIOMETRIC].gnd = read_2; -err: - if (ret) - dev_err(vadc->dev, "measure reference points failed\n"); return ret; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4] iio: vadc: Fix potential dereference of NULL pointer 2022-05-22 16:53 ` [PATCH v4] " Yongzhi Liu @ 2022-06-03 15:02 ` Jonathan Cameron 2022-06-03 15:20 ` Jonathan Cameron 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Cameron @ 2022-06-03 15:02 UTC (permalink / raw) To: Yongzhi Liu Cc: agross, bjorn.andersson, lars, svarbanov, iivanov, jonathan.cameron, linux-arm-msm, linux-iio, linux-kernel, fuyq On Sun, 22 May 2022 09:53:47 -0700 Yongzhi Liu <lyz_cs@pku.edu.cn> wrote: > The return value of vadc_get_channel() needs to be checked to > avoid use of NULL pointer. vadc_do_conversion() already provides > error prints in at least some of it's error paths. Thus it is > reasonable to add the null pointer check on prop and drop the > extra reporting in vadc_measure_ref_points(). > > Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> Hi Biggest remaining thing is squashing ret = -ENODEV; return ret; into the shorter return -ENODEV; > --- > drivers/iio/adc/qcom-spmi-vadc.c | 38 ++++++++++++++++++++++++++++---------- > 1 file changed, 28 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c > index 34202ba..43a52b1 100644 > --- a/drivers/iio/adc/qcom-spmi-vadc.c > +++ b/drivers/iio/adc/qcom-spmi-vadc.c > @@ -358,22 +358,33 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) > vadc->graph[VADC_CALIB_ABSOLUTE].dx = VADC_ABSOLUTE_RANGE_UV; > > prop = vadc_get_channel(vadc, VADC_REF_1250MV); > + if (!prop) { > + dev_err(vadc->dev, "Please define 1.25V channel\n"); Probably makes more sense to have the error as "No 1.25V channel found\n"); It's not obvious to anyone getting this error what 'define' might mean without them looking at the code, so I'd rather we just said what had gone wrong rather offering incomplete advice. > + ret = -ENODEV; Don't bother assigning a variable just to return it in the next line. return -ENODEV; > + return ret; > + } > ret = vadc_do_conversion(vadc, prop, &read_1); > if (ret) > - goto err; > + return ret; > > /* Try with buffered 625mV channel first */ > prop = vadc_get_channel(vadc, VADC_SPARE1); > - if (!prop) > + if (!prop) { > prop = vadc_get_channel(vadc, VADC_REF_625MV); > + if (!prop) { > + dev_err(vadc->dev, "Please define 0.625V channel\n"); "No 0.625V channel found\n" > + ret = -ENODEV; return -ENODEV; > + return ret; > + } > + } > > ret = vadc_do_conversion(vadc, prop, &read_2); > if (ret) > - goto err; > + return ret; > > if (read_1 == read_2) { > ret = -EINVAL; > - goto err; > + return ret; > } > > vadc->graph[VADC_CALIB_ABSOLUTE].dy = read_1 - read_2; > @@ -381,25 +392,32 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) > > /* Ratiometric calibration */ > prop = vadc_get_channel(vadc, VADC_VDD_VADC); > + if (!prop) { > + dev_err(vadc->dev, "Please define VDD channel\n"); "No VDD channel found\n" > + ret = -ENODEV; > + return ret; > + } > ret = vadc_do_conversion(vadc, prop, &read_1); > if (ret) > - goto err; > + return ret; > > prop = vadc_get_channel(vadc, VADC_GND_REF); > + if (!prop) { > + dev_err(vadc->dev, "Please define GND channel\n"); "No GND channel found\n" > + ret = -ENODEV; > + return ret; return -ENODEV; > + } > ret = vadc_do_conversion(vadc, prop, &read_2); > if (ret) > - goto err; > + return ret; > > if (read_1 == read_2) { > ret = -EINVAL; > - goto err; > + return ret; return -ENODEV; > } > > vadc->graph[VADC_CALIB_RATIOMETRIC].dy = read_1 - read_2; > vadc->graph[VADC_CALIB_RATIOMETRIC].gnd = read_2; > -err: > - if (ret) > - dev_err(vadc->dev, "measure reference points failed\n"); > > return ret; Can't get here with anything other than ret == 0 so return 0; to make that explicit. > } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] iio: vadc: Fix potential dereference of NULL pointer 2022-06-03 15:02 ` Jonathan Cameron @ 2022-06-03 15:20 ` Jonathan Cameron 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Cameron @ 2022-06-03 15:20 UTC (permalink / raw) To: Yongzhi Liu Cc: agross, bjorn.andersson, lars, svarbanov, iivanov, jonathan.cameron, linux-arm-msm, linux-iio, linux-kernel, fuyq On Fri, 3 Jun 2022 16:02:22 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Sun, 22 May 2022 09:53:47 -0700 > Yongzhi Liu <lyz_cs@pku.edu.cn> wrote: > > > The return value of vadc_get_channel() needs to be checked to > > avoid use of NULL pointer. vadc_do_conversion() already provides > > error prints in at least some of it's error paths. Thus it is > > reasonable to add the null pointer check on prop and drop the > > extra reporting in vadc_measure_ref_points(). > > > > Signed-off-by: Yongzhi Liu <lyz_cs@pku.edu.cn> > > Hi > > Biggest remaining thing is squashing > ret = -ENODEV; > return ret; > > into the shorter > return -ENODEV; > One additional process thing I didn't mention before now as this is a single patch. Generally for IIO at least, don't send new versions in reply to old threads. The threads can get very deep and confusing, so I'd much rather a new thread for each version. Thanks, Jonathan > > > --- > > drivers/iio/adc/qcom-spmi-vadc.c | 38 ++++++++++++++++++++++++++++---------- > > 1 file changed, 28 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c > > index 34202ba..43a52b1 100644 > > --- a/drivers/iio/adc/qcom-spmi-vadc.c > > +++ b/drivers/iio/adc/qcom-spmi-vadc.c > > @@ -358,22 +358,33 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) > > vadc->graph[VADC_CALIB_ABSOLUTE].dx = VADC_ABSOLUTE_RANGE_UV; > > > > prop = vadc_get_channel(vadc, VADC_REF_1250MV); > > + if (!prop) { > > + dev_err(vadc->dev, "Please define 1.25V channel\n"); > Probably makes more sense to have the error as > "No 1.25V channel found\n"); > > It's not obvious to anyone getting this error what 'define' might mean > without them looking at the code, so I'd rather we just said what had > gone wrong rather offering incomplete advice. > > > + ret = -ENODEV; > > Don't bother assigning a variable just to return it in the next line. > > return -ENODEV; > > > + return ret; > > + } > > ret = vadc_do_conversion(vadc, prop, &read_1); > > if (ret) > > - goto err; > > + return ret; > > > > /* Try with buffered 625mV channel first */ > > prop = vadc_get_channel(vadc, VADC_SPARE1); > > - if (!prop) > > + if (!prop) { > > prop = vadc_get_channel(vadc, VADC_REF_625MV); > > + if (!prop) { > > + dev_err(vadc->dev, "Please define 0.625V channel\n"); > "No 0.625V channel found\n" > > + ret = -ENODEV; > > return -ENODEV; > > > + return ret; > > + } > > + } > > > > ret = vadc_do_conversion(vadc, prop, &read_2); > > if (ret) > > - goto err; > > + return ret; > > > > if (read_1 == read_2) { > > ret = -EINVAL; > > - goto err; > > + return ret; > > } > > > > vadc->graph[VADC_CALIB_ABSOLUTE].dy = read_1 - read_2; > > @@ -381,25 +392,32 @@ static int vadc_measure_ref_points(struct vadc_priv *vadc) > > > > /* Ratiometric calibration */ > > prop = vadc_get_channel(vadc, VADC_VDD_VADC); > > + if (!prop) { > > + dev_err(vadc->dev, "Please define VDD channel\n"); > > "No VDD channel found\n" > > > + ret = -ENODEV; > > + return ret; > > + } > > ret = vadc_do_conversion(vadc, prop, &read_1); > > if (ret) > > - goto err; > > + return ret; > > > > prop = vadc_get_channel(vadc, VADC_GND_REF); > > + if (!prop) { > > + dev_err(vadc->dev, "Please define GND channel\n"); > > "No GND channel found\n" > > > + ret = -ENODEV; > > + return ret; > > return -ENODEV; > > > + } > > ret = vadc_do_conversion(vadc, prop, &read_2); > > if (ret) > > - goto err; > > + return ret; > > > > if (read_1 == read_2) { > > ret = -EINVAL; > > - goto err; > > + return ret; > > return -ENODEV; > > > } > > > > vadc->graph[VADC_CALIB_RATIOMETRIC].dy = read_1 - read_2; > > vadc->graph[VADC_CALIB_RATIOMETRIC].gnd = read_2; > > -err: > > - if (ret) > > - dev_err(vadc->dev, "measure reference points failed\n"); > > > > return ret; > > Can't get here with anything other than ret == 0 so > return 0; > to make that explicit. > > > > } > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-06-03 15:11 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-18 6:43 [PATCH] iio: vadc: Fix potential dereference of NULL pointer Yongzhi Liu 2022-05-18 17:31 ` Matthias Kaehlcke 2022-05-19 5:50 ` [PATCH v2] " Yongzhi Liu 2022-05-20 17:13 ` Jonathan Cameron 2022-05-21 3:31 ` [PATCH] hv_netvsc: " Yongzhi Liu 2022-05-21 3:34 ` 刘永志 2022-05-23 15:21 ` Andy Shevchenko 2022-05-23 15:45 ` 刘永志 2022-05-21 3:35 ` [PATCH v3] iio: vadc: " Yongzhi Liu 2022-05-22 11:01 ` Jonathan Cameron 2022-05-22 16:53 ` [PATCH v4] " Yongzhi Liu 2022-06-03 15:02 ` Jonathan Cameron 2022-06-03 15:20 ` Jonathan Cameron
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.