All of lore.kernel.org
 help / color / mirror / Atom feed
* [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	[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	[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	[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

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

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