[PATCH 1/2] media: pxa_camera: don't deferenciate a NULL pointer

[PATCH 1/2] media: pxa_camera: don't deferenciate a NULL pointer

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/platform/pxa_camera.c:2400 pxa_camera_probe() error: we previously assumed 'pcdev->pdata' could be null (see line 2397)

It would be possible that neither DT nor platform data would be
provided. This is a Kernel bug, so warn about that and bail.

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/platform/pxa_camera.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index 5f930560eb30..f91f8fd424c4 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -2396,6 +2396,9 @@ static int pxa_camera_probe(struct platform_device *pdev)
 	pcdev->pdata = pdev->dev.platform_data;
 	if (pdev->dev.of_node && !pcdev->pdata) {
 		err = pxa_camera_pdata_from_dt(&pdev->dev, pcdev, &pcdev->asd);
+	} else if (!pcdev->pdata) {
+		WARN_ON(1);
+		return -ENODEV;
 	} else {
 		pcdev->platform_flags = pcdev->pdata->flags;
 		pcdev->mclk = pcdev->pdata->mclk_10khz * 10000;
-- 
2.19.2

[PATCH 2/2] media: drxk_hard: check if parameter is not NULL

[Mauro Carvalho Chehab]

There is a smatch warning:
	drivers/media/dvb-frontends/drxk_hard.c: drivers/media/dvb-frontends/drxk_hard.c:1478 scu_command() error: we previously assumed 'parameter' could be null (see line 1467)

Telling that parameter might be NULL. Well, it can't, due to the
way the driver works, but it doesn't hurt to add a check, in order
to shut up smatch.

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/dvb-frontends/drxk_hard.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/drxk_hard.c b/drivers/media/dvb-frontends/drxk_hard.c
index 84ac3f73f8fe..8ea1e45be710 100644
--- a/drivers/media/dvb-frontends/drxk_hard.c
+++ b/drivers/media/dvb-frontends/drxk_hard.c
@@ -1474,9 +1474,11 @@ static int scu_command(struct drxk_state *state,
 
 	/* assume that the command register is ready
 		since it is checked afterwards */
-	for (ii = parameter_len - 1; ii >= 0; ii -= 1) {
-		buffer[cnt++] = (parameter[ii] & 0xFF);
-		buffer[cnt++] = ((parameter[ii] >> 8) & 0xFF);
+	if (parameter) {
+		for (ii = parameter_len - 1; ii >= 0; ii -= 1) {
+			buffer[cnt++] = (parameter[ii] & 0xFF);
+			buffer[cnt++] = ((parameter[ii] >> 8) & 0xFF);
+		}
 	}
 	buffer[cnt++] = (cmd & 0xFF);
 	buffer[cnt++] = ((cmd >> 8) & 0xFF);
-- 
2.19.2

Re: [PATCH 2/2] media: drxk_hard: check if parameter is not NULL

[Nick Desaulniers]

On Fri, Dec 7, 2018 at 5:08 AM Mauro Carvalho Chehab
<mchehab+samsung@kernel.org> wrote:
>
> There is a smatch warning:
>         drivers/media/dvb-frontends/drxk_hard.c: drivers/media/dvb-frontends/drxk_hard.c:1478 scu_command() error: we previously assumed 'parameter' could be null (see line 1467)
>
> Telling that parameter might be NULL. Well, it can't, due to the
> way the driver works, but it doesn't hurt to add a check, in order
> to shut up smatch.

eh, yeah this function is kind of odd; the early return conditions are
a little tricky, but I agree that this check doesn't hurt to add.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
>  drivers/media/dvb-frontends/drxk_hard.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/drxk_hard.c b/drivers/media/dvb-frontends/drxk_hard.c
> index 84ac3f73f8fe..8ea1e45be710 100644
> --- a/drivers/media/dvb-frontends/drxk_hard.c
> +++ b/drivers/media/dvb-frontends/drxk_hard.c
> @@ -1474,9 +1474,11 @@ static int scu_command(struct drxk_state *state,
>
>         /* assume that the command register is ready
>                 since it is checked afterwards */
> -       for (ii = parameter_len - 1; ii >= 0; ii -= 1) {
> -               buffer[cnt++] = (parameter[ii] & 0xFF);
> -               buffer[cnt++] = ((parameter[ii] >> 8) & 0xFF);
> +       if (parameter) {
> +               for (ii = parameter_len - 1; ii >= 0; ii -= 1) {
> +                       buffer[cnt++] = (parameter[ii] & 0xFF);
> +                       buffer[cnt++] = ((parameter[ii] >> 8) & 0xFF);
> +               }
>         }
>         buffer[cnt++] = (cmd & 0xFF);
>         buffer[cnt++] = ((cmd >> 8) & 0xFF);
> --
> 2.19.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 1/2] media: pxa_camera: don't deferenciate a NULL pointer

[Sakari Ailus]

Hi Mauro,

On Fri, Dec 07, 2018 at 08:07:54AM -0500, Mauro Carvalho Chehab wrote:
> As warned by smatch:
> 	drivers/media/platform/pxa_camera.c:2400 pxa_camera_probe() error: we previously assumed 'pcdev->pdata' could be null (see line 2397)
> 
> It would be possible that neither DT nor platform data would be
> provided. This is a Kernel bug, so warn about that and bail.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
>  drivers/media/platform/pxa_camera.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> index 5f930560eb30..f91f8fd424c4 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -2396,6 +2396,9 @@ static int pxa_camera_probe(struct platform_device *pdev)
>  	pcdev->pdata = pdev->dev.platform_data;
>  	if (pdev->dev.of_node && !pcdev->pdata) {
>  		err = pxa_camera_pdata_from_dt(&pdev->dev, pcdev, &pcdev->asd);
> +	} else if (!pcdev->pdata) {

This fixes the issue, but the current checks remain a bit odd.

The driver seems to prefer platform data over OF. I wonder if that's
intentional or not.

In that case, I'd roughly write this as:

if (pcdev->pdata) {
	...;
} else if (pdev->dev.of_node) {
	...;
} else {
	return -ENODEV;
}

I'm not sure WARN_ON(1) is necessary. A lot of drivers simply do it this
way without WARN_ON().

> +		WARN_ON(1);
> +		return -ENODEV;
>  	} else {
>  		pcdev->platform_flags = pcdev->pdata->flags;
>  		pcdev->mclk = pcdev->pdata->mclk_10khz * 10000;

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com