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
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.
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
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
Yongzhi Liu <lyz_cs@pku.edu.cn> writes: > 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(-) Did you sent this to linux-wireless by mistake? -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> -----Original Messages----- > From: "Kalle Valo" <kvalo@kernel.org> > Sent Time: 2022-05-19 23:30:51 (Thursday) > To: "Yongzhi Liu" <lyz_cs@pku.edu.cn> > Cc: amitkarwar@gmail.com, ganapathi017@gmail.com, sharvari.harisangam@nxp.com, huxinming820@gmail.com, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, arend.vanspriel@broadcom.com, linux-wireless@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, fuyq@stu.pku.edu.cn > Subject: Re: [PATCH v2] iio: vadc: Fix potential dereference of NULL pointer > > Yongzhi Liu <lyz_cs@pku.edu.cn> writes: > > > 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(-) > I'm sorry to send this to linux-wireless by mistake. I will cautiously submit patches later. > Did you sent this to linux-wireless by mistake? > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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;
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
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
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
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; > }
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
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
> -----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
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. > }
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. > > > > } >