[PATCH 0/6] Old non-MC-aware drivers have no sub-device state

[PATCH 0/6] Old non-MC-aware drivers have no sub-device state

[Sakari Ailus]

Hi folks,

This set replaces the earlier attempt to fix non-MC-aware sub-device
drivers that still use the set_fmt and similar sub-device ops. These
drivers have been converted from the olf set_fmt etc. video ops.

The issue here is that the caller does not initialise the full sub-device
state so the sd field of struct v4l2_subdev_state is NULL, leading
currently to NULL pointer dereference, even if the code compiles. This was
not the case before commit fd17e3a9a7886ec949ce269a396b67875b51ff45 .

Even then, there's no need to access the sub-device state as the format
(or selection rectangle) won't be stored for a longer period of time: the
caller (saa7134 driver) simply uses the original configuration to obtain
the changed value.

This patchset does not address similar issues in the ov6650 driver.

Sakari Ailus (6):
  media: saa6752hs: Don't set format in sub-device state
  media: adv7183: Don't set format in sub-device state
  media: mt9t112: Don't set format in sub-device state
  media: rj54n1cb0c: Don't set format in sub-device state
  media: tw9910: Don't set format in sub-device state
  media: ov9640: Don't set format in sub-device state

 drivers/media/i2c/adv7183.c    | 2 --
 drivers/media/i2c/mt9t112.c    | 1 -
 drivers/media/i2c/ov9640.c     | 2 --
 drivers/media/i2c/rj54n1cb0c.c | 4 +---
 drivers/media/i2c/saa6752hs.c  | 4 +---
 drivers/media/i2c/tw9910.c     | 2 --
 6 files changed, 2 insertions(+), 13 deletions(-)

-- 
2.39.2

[PATCH 1/6] media: saa6752hs: Don't set format in sub-device state

[Sakari Ailus]

For the purpose of setting old non-pad based sub-device try format as a
basis for VIDIOC_TRY_FMT implementation, there is no need to set the
format in the sub-device state. Drop the assignment to the state, which
would result in a NULL pointer dereference.

Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/saa6752hs.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/i2c/saa6752hs.c b/drivers/media/i2c/saa6752hs.c
index 51b62a97946a..897eaa669b86 100644
--- a/drivers/media/i2c/saa6752hs.c
+++ b/drivers/media/i2c/saa6752hs.c
@@ -594,10 +594,8 @@ static int saa6752hs_set_fmt(struct v4l2_subdev *sd,
 	f->field = V4L2_FIELD_INTERLACED;
 	f->colorspace = V4L2_COLORSPACE_SMPTE170M;
 
-	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		*v4l2_subdev_state_get_format(sd_state, 0) = *f;
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
 		return 0;
-	}
 
 	/*
 	  FIXME: translate and round width/height into EMPRESS
-- 
2.39.2

[PATCH 2/6] media: adv7183: Don't set format in sub-device state

[Sakari Ailus]

For the purpose of setting old non-pad based sub-device try format as a
basis for VIDIOC_TRY_FMT implementation, there is no need to set the
format in the sub-device state. Drop the assignment to the state, which
would result in a NULL pointer dereference.

Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/adv7183.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/i2c/adv7183.c b/drivers/media/i2c/adv7183.c
index bcb99ba9a272..2a2cace4a153 100644
--- a/drivers/media/i2c/adv7183.c
+++ b/drivers/media/i2c/adv7183.c
@@ -442,8 +442,6 @@ static int adv7183_set_fmt(struct v4l2_subdev *sd,
 	}
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		decoder->fmt = *fmt;
-	else
-		*v4l2_subdev_state_get_format(sd_state, 0) = *fmt;
 	return 0;
 }
 
-- 
2.39.2

[PATCH 3/6] media: mt9t112: Don't set format in sub-device state

[Sakari Ailus]

For the purpose of setting old non-pad based sub-device try format as a
basis for VIDIOC_TRY_FMT implementation, there is no need to set the
format in the sub-device state. Drop the assignment to the state, which
would result in a NULL pointer dereference.

Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/mt9t112.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/mt9t112.c b/drivers/media/i2c/mt9t112.c
index 2e2d9853c089..fb1588c57cc8 100644
--- a/drivers/media/i2c/mt9t112.c
+++ b/drivers/media/i2c/mt9t112.c
@@ -982,7 +982,6 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		return mt9t112_s_fmt(sd, mf);
-	*v4l2_subdev_state_get_format(sd_state, 0) = *mf;
 
 	return 0;
 }
-- 
2.39.2

[PATCH 4/6] media: rj54n1cb0c: Don't set format in sub-device state

[Sakari Ailus]

For the purpose of setting old non-pad based sub-device try format as a
basis for VIDIOC_TRY_FMT implementation, there is no need to set the
format in the sub-device state. Drop the assignment to the state, which
would result in a NULL pointer dereference.

Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/rj54n1cb0c.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/i2c/rj54n1cb0c.c b/drivers/media/i2c/rj54n1cb0c.c
index 403185b18f06..a59db10153cd 100644
--- a/drivers/media/i2c/rj54n1cb0c.c
+++ b/drivers/media/i2c/rj54n1cb0c.c
@@ -1008,10 +1008,8 @@ static int rj54n1_set_fmt(struct v4l2_subdev *sd,
 	v4l_bound_align_image(&mf->width, 112, RJ54N1_MAX_WIDTH, align,
 			      &mf->height, 84, RJ54N1_MAX_HEIGHT, align, 0);
 
-	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		*v4l2_subdev_state_get_format(sd_state, 0) = *mf;
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
 		return 0;
-	}
 
 	/*
 	 * Verify if the sensor has just been powered on. TODO: replace this
-- 
2.39.2

[PATCH 5/6] media: tw9910: Don't set format in sub-device state

[Sakari Ailus]

For the purpose of setting old non-pad based sub-device try format as a
basis for VIDIOC_TRY_FMT implementation, there is no need to set the
format in the sub-device state. Drop the assignment to the state, which
would result in a NULL pointer dereference.

Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/tw9910.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
index 7c331a7f12d4..905af98c7d53 100644
--- a/drivers/media/i2c/tw9910.c
+++ b/drivers/media/i2c/tw9910.c
@@ -829,8 +829,6 @@ static int tw9910_set_fmt(struct v4l2_subdev *sd,
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		return tw9910_s_fmt(sd, mf);
 
-	*v4l2_subdev_state_get_format(sd_state, 0) = *mf;
-
 	return 0;
 }
 
-- 
2.39.2

[PATCH 6/6] media: ov9640: Don't set format in sub-device state

[Sakari Ailus]

For the purpose of setting old non-pad based sub-device try format as a
basis for VIDIOC_TRY_FMT implementation, there is no need to set the
format in the sub-device state. Drop the assignment to the state, which
would result in a NULL pointer dereference.

Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ov9640.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
index b0c171fe75bc..e9a52a8a9dc0 100644
--- a/drivers/media/i2c/ov9640.c
+++ b/drivers/media/i2c/ov9640.c
@@ -547,8 +547,6 @@ static int ov9640_set_fmt(struct v4l2_subdev *sd,
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		return ov9640_s_fmt(sd, mf);
 
-	*v4l2_subdev_state_get_format(sd_state, 0) = *mf;
-
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH 1/6] media: saa6752hs: Don't set format in sub-device state

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Dec 07, 2023 at 02:09:07PM +0200, Sakari Ailus wrote:
> For the purpose of setting old non-pad based sub-device try format as a
> basis for VIDIOC_TRY_FMT implementation, there is no need to set the
> format in the sub-device state. Drop the assignment to the state, which
> would result in a NULL pointer dereference.
> 
> Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/saa6752hs.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/saa6752hs.c b/drivers/media/i2c/saa6752hs.c
> index 51b62a97946a..897eaa669b86 100644
> --- a/drivers/media/i2c/saa6752hs.c
> +++ b/drivers/media/i2c/saa6752hs.c
> @@ -594,10 +594,8 @@ static int saa6752hs_set_fmt(struct v4l2_subdev *sd,
>  	f->field = V4L2_FIELD_INTERLACED;
>  	f->colorspace = V4L2_COLORSPACE_SMPTE170M;
>  
> -	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		*v4l2_subdev_state_get_format(sd_state, 0) = *f;
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>  		return 0;
> -	}
>  
>  	/*
>  	  FIXME: translate and round width/height into EMPRESS

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] media: adv7183: Don't set format in sub-device state

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Dec 07, 2023 at 02:09:08PM +0200, Sakari Ailus wrote:
> For the purpose of setting old non-pad based sub-device try format as a
> basis for VIDIOC_TRY_FMT implementation, there is no need to set the
> format in the sub-device state. Drop the assignment to the state, which
> would result in a NULL pointer dereference.
> 
> Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/adv7183.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7183.c b/drivers/media/i2c/adv7183.c
> index bcb99ba9a272..2a2cace4a153 100644
> --- a/drivers/media/i2c/adv7183.c
> +++ b/drivers/media/i2c/adv7183.c
> @@ -442,8 +442,6 @@ static int adv7183_set_fmt(struct v4l2_subdev *sd,
>  	}
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		decoder->fmt = *fmt;
> -	else
> -		*v4l2_subdev_state_get_format(sd_state, 0) = *fmt;
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] media: mt9t112: Don't set format in sub-device state

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Dec 07, 2023 at 02:09:09PM +0200, Sakari Ailus wrote:
> For the purpose of setting old non-pad based sub-device try format as a
> basis for VIDIOC_TRY_FMT implementation, there is no need to set the
> format in the sub-device state. Drop the assignment to the state, which
> would result in a NULL pointer dereference.
> 
> Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/mt9t112.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/mt9t112.c b/drivers/media/i2c/mt9t112.c
> index 2e2d9853c089..fb1588c57cc8 100644
> --- a/drivers/media/i2c/mt9t112.c
> +++ b/drivers/media/i2c/mt9t112.c
> @@ -982,7 +982,6 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return mt9t112_s_fmt(sd, mf);
> -	*v4l2_subdev_state_get_format(sd_state, 0) = *mf;
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] media: rj54n1cb0c: Don't set format in sub-device state

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Dec 07, 2023 at 02:09:10PM +0200, Sakari Ailus wrote:
> For the purpose of setting old non-pad based sub-device try format as a
> basis for VIDIOC_TRY_FMT implementation, there is no need to set the
> format in the sub-device state. Drop the assignment to the state, which
> would result in a NULL pointer dereference.
> 
> Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/rj54n1cb0c.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/rj54n1cb0c.c b/drivers/media/i2c/rj54n1cb0c.c
> index 403185b18f06..a59db10153cd 100644
> --- a/drivers/media/i2c/rj54n1cb0c.c
> +++ b/drivers/media/i2c/rj54n1cb0c.c
> @@ -1008,10 +1008,8 @@ static int rj54n1_set_fmt(struct v4l2_subdev *sd,
>  	v4l_bound_align_image(&mf->width, 112, RJ54N1_MAX_WIDTH, align,
>  			      &mf->height, 84, RJ54N1_MAX_HEIGHT, align, 0);
>  
> -	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		*v4l2_subdev_state_get_format(sd_state, 0) = *mf;
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>  		return 0;
> -	}
>  
>  	/*
>  	 * Verify if the sensor has just been powered on. TODO: replace this

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] media: tw9910: Don't set format in sub-device state

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Dec 07, 2023 at 02:09:11PM +0200, Sakari Ailus wrote:
> For the purpose of setting old non-pad based sub-device try format as a
> basis for VIDIOC_TRY_FMT implementation, there is no need to set the
> format in the sub-device state. Drop the assignment to the state, which
> would result in a NULL pointer dereference.
> 
> Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/tw9910.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
> index 7c331a7f12d4..905af98c7d53 100644
> --- a/drivers/media/i2c/tw9910.c
> +++ b/drivers/media/i2c/tw9910.c
> @@ -829,8 +829,6 @@ static int tw9910_set_fmt(struct v4l2_subdev *sd,
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return tw9910_s_fmt(sd, mf);
>  
> -	*v4l2_subdev_state_get_format(sd_state, 0) = *mf;
> -
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/6] media: ov9640: Don't set format in sub-device state

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Dec 07, 2023 at 02:09:12PM +0200, Sakari Ailus wrote:
> For the purpose of setting old non-pad based sub-device try format as a
> basis for VIDIOC_TRY_FMT implementation, there is no need to set the
> format in the sub-device state. Drop the assignment to the state, which
> would result in a NULL pointer dereference.
> 
> Fixes: fd17e3a9a788 ("media: i2c: Use accessors for pad config 'try_*' fields")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/ov9640.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
> index b0c171fe75bc..e9a52a8a9dc0 100644
> --- a/drivers/media/i2c/ov9640.c
> +++ b/drivers/media/i2c/ov9640.c
> @@ -547,8 +547,6 @@ static int ov9640_set_fmt(struct v4l2_subdev *sd,
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return ov9640_s_fmt(sd, mf);
>  
> -	*v4l2_subdev_state_get_format(sd_state, 0) = *mf;
> -
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/6] Old non-MC-aware drivers have no sub-device state

[Laurent Pinchart]

On Thu, Dec 07, 2023 at 02:09:06PM +0200, Sakari Ailus wrote:
> Hi folks,
> 
> This set replaces the earlier attempt to fix non-MC-aware sub-device
> drivers that still use the set_fmt and similar sub-device ops. These
> drivers have been converted from the olf set_fmt etc. video ops.
> 
> The issue here is that the caller does not initialise the full sub-device
> state so the sd field of struct v4l2_subdev_state is NULL, leading
> currently to NULL pointer dereference, even if the code compiles. This was
> not the case before commit fd17e3a9a7886ec949ce269a396b67875b51ff45 .
> 
> Even then, there's no need to access the sub-device state as the format
> (or selection rectangle) won't be stored for a longer period of time: the
> caller (saa7134 driver) simply uses the original configuration to obtain
> the changed value.

Just a note that he caller for the other subdev drivers, renesas-ceu,
does exactly the same thing, so the same fix is applicable.

> This patchset does not address similar issues in the ov6650 driver.

The driver is not used in mainline anymore. It was merged for an old
OMAP1 device that has been dropped from the kernel.

I wonder if we should start moving some sensor drivers to staging...

> Sakari Ailus (6):
>   media: saa6752hs: Don't set format in sub-device state
>   media: adv7183: Don't set format in sub-device state
>   media: mt9t112: Don't set format in sub-device state
>   media: rj54n1cb0c: Don't set format in sub-device state
>   media: tw9910: Don't set format in sub-device state
>   media: ov9640: Don't set format in sub-device state
> 
>  drivers/media/i2c/adv7183.c    | 2 --
>  drivers/media/i2c/mt9t112.c    | 1 -
>  drivers/media/i2c/ov9640.c     | 2 --
>  drivers/media/i2c/rj54n1cb0c.c | 4 +---
>  drivers/media/i2c/saa6752hs.c  | 4 +---
>  drivers/media/i2c/tw9910.c     | 2 --
>  6 files changed, 2 insertions(+), 13 deletions(-)

-- 
Regards,

Laurent Pinchart