[PATCH v2 0/2] Fix potential buffer overrun root cause

[PATCH v2 0/2] Fix potential buffer overrun root cause

[Niklas Söderlund]

Hi,

Commit 015060cb7795eac3 ("media: rcar-vin: enable field toggle after a
set number of lines for Gen3") was an attempt to fix the issue of
writing outside the capture buffer for VIN Gen3. Unfortunately it only
fixed a symptom of a problem to such a degree I could no longer
reproduce it.

Jacopo on the other hand working on a different setup still ran into the
issue. And he even figured out the root cause of the issue. When I
submitted the original VIN Gen3 support I had when addressing a review
comment missed to keep the crop and compose dimensions in sync with the
requested format resulting in the DMA engine not properly stopping
before writing outside the buffer.

This series reverts the incorrect fix in 1/2 and applies a correct one
in 2/2. I think this should be picked up for v4.18.

* Changes since v1
- Add commit message to 1/2.

Niklas Söderlund (2):
  Revert "media: rcar-vin: enable field toggle after a set number of
    lines for Gen3"
  rcar-vin: fix crop and compose handling for Gen3

 drivers/media/platform/rcar-vin/rcar-dma.c  | 20 +++++---------------
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  6 ++++++
 2 files changed, 11 insertions(+), 15 deletions(-)

-- 
2.17.0

[PATCH v2 1/2] Revert "media: rcar-vin: enable field toggle after a set number of lines for Gen3"

[Niklas Söderlund]

The offending commit was an attempt to fix the issue of writing outside
the capture buffer for VIN Gen3. Unfortunately it only fixed the symptom
of the problem to such a degree I could no longer reproduce it. Revert
the offending commit before a proper fix can be added in a follow-up
patch.

This reverts commit 015060cb7795eac34454696cc9c9f8b76926a401.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index b41ba9a4a2b3ac90..ac07f99e3516a620 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -124,9 +124,7 @@
 #define VNDMR2_VPS		(1 << 30)
 #define VNDMR2_HPS		(1 << 29)
 #define VNDMR2_FTEV		(1 << 17)
-#define VNDMR2_FTEH		(1 << 16)
 #define VNDMR2_VLV(n)		((n & 0xf) << 12)
-#define VNDMR2_HLV(n)		((n) & 0xfff)
 
 /* Video n CSI2 Interface Mode Register (Gen3) */
 #define VNCSI_IFMD_DES1		(1 << 26)
@@ -614,9 +612,8 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
 
 static int rvin_setup(struct rvin_dev *vin)
 {
-	u32 vnmc, dmr, dmr2, interrupts, lines;
+	u32 vnmc, dmr, dmr2, interrupts;
 	bool progressive = false, output_is_yuv = false, input_is_yuv = false;
-	bool halfsize = false;
 
 	switch (vin->format.field) {
 	case V4L2_FIELD_TOP:
@@ -631,15 +628,12 @@ static int rvin_setup(struct rvin_dev *vin)
 		/* Use BT if video standard can be read and is 60 Hz format */
 		if (!vin->info->use_mc && vin->std & V4L2_STD_525_60)
 			vnmc = VNMC_IM_FULL | VNMC_FOC;
-		halfsize = true;
 		break;
 	case V4L2_FIELD_INTERLACED_TB:
 		vnmc = VNMC_IM_FULL;
-		halfsize = true;
 		break;
 	case V4L2_FIELD_INTERLACED_BT:
 		vnmc = VNMC_IM_FULL | VNMC_FOC;
-		halfsize = true;
 		break;
 	case V4L2_FIELD_NONE:
 		vnmc = VNMC_IM_ODD_EVEN;
@@ -682,15 +676,11 @@ static int rvin_setup(struct rvin_dev *vin)
 		break;
 	}
 
-	if (vin->info->model == RCAR_GEN3) {
-		/* Enable HSYNC Field Toggle mode after height HSYNC inputs. */
-		lines = vin->format.height / (halfsize ? 2 : 1);
-		dmr2 = VNDMR2_FTEH | VNDMR2_HLV(lines);
-		vin_dbg(vin, "Field Toogle after %u lines\n", lines);
-	} else {
-		/* Enable VSYNC Field Toogle mode after one VSYNC input. */
+	/* Enable VSYNC Field Toogle mode after one VSYNC input */
+	if (vin->info->model == RCAR_GEN3)
+		dmr2 = VNDMR2_FTEV;
+	else
 		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
-	}
 
 	/* Hsync Signal Polarity Select */
 	if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
-- 
2.17.0

[PATCH v2 2/2] rcar-vin: fix crop and compose handling for Gen3

[Niklas Söderlund]

When refactoring the Gen3 enablement series crop and compose handling
where broken. This went unnoticed but can result in writing out side the
capture buffer. Fix this by restoring the crop and compose to reflect
the format dimensions as we have not yet enabled the scaler for Gen3.

Fixes: 5e7c623632fcf8f5 ("media: rcar-vin: use different v4l2 operations in media controller mode")
Reported-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 2fb8587116f25a4f..e78fba84d59028ef 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -702,6 +702,12 @@ static int rvin_mc_s_fmt_vid_cap(struct file *file, void *priv,
 
 	vin->format = f->fmt.pix;
 
+	vin->crop.top = 0;
+	vin->crop.left = 0;
+	vin->crop.width = vin->format.width;
+	vin->crop.height = vin->format.height;
+	vin->compose = vin->crop;
+
 	return 0;
 }
 
-- 
2.17.0

Re: [PATCH v2 2/2] rcar-vin: fix crop and compose handling for Gen3

[Sergei Shtylyov]

Hello!

On 05/11/2018 05:41 PM, Niklas Söderlund wrote:

> When refactoring the Gen3 enablement series crop and compose handling
> where broken. This went unnoticed but can result in writing out side the

   s/Where/Were/?

> capture buffer. Fix this by restoring the crop and compose to reflect
> the format dimensions as we have not yet enabled the scaler for Gen3.
> 
> Fixes: 5e7c623632fcf8f5 ("media: rcar-vin: use different v4l2 operations in media controller mode")
> Reported-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

Re: [PATCH v2 0/2] Fix potential buffer overrun root cause

[Simon Horman]

On Fri, May 11, 2018 at 04:41:24PM +0200, Niklas Söderlund wrote:
> Hi,
> 
> Commit 015060cb7795eac3 ("media: rcar-vin: enable field toggle after a
> set number of lines for Gen3") was an attempt to fix the issue of
> writing outside the capture buffer for VIN Gen3. Unfortunately it only
> fixed a symptom of a problem to such a degree I could no longer
> reproduce it.
> 
> Jacopo on the other hand working on a different setup still ran into the
> issue. And he even figured out the root cause of the issue. When I
> submitted the original VIN Gen3 support I had when addressing a review
> comment missed to keep the crop and compose dimensions in sync with the
> requested format resulting in the DMA engine not properly stopping
> before writing outside the buffer.
> 
> This series reverts the incorrect fix in 1/2 and applies a correct one
> in 2/2. I think this should be picked up for v4.18.
> 
> * Changes since v1
> - Add commit message to 1/2.
> 
> Niklas Söderlund (2):
>   Revert "media: rcar-vin: enable field toggle after a set number of
>     lines for Gen3"
>   rcar-vin: fix crop and compose handling for Gen3

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH v2 2/2] rcar-vin: fix crop and compose handling for Gen3

[Kieran Bingham]

Hi Niklas,

This looks like quite the improvement :D

On 11/05/18 15:41, Niklas Söderlund wrote:
> When refactoring the Gen3 enablement series crop and compose handling
> where broken. This went unnoticed but can result in writing out side the

As well as Sergei's 'where/were', 'out side' is one word in this context.

'outside of the capture buffer'

> capture buffer. Fix this by restoring the crop and compose to reflect
> the format dimensions as we have not yet enabled the scaler for Gen3.
> 
> Fixes: 5e7c623632fcf8f5 ("media: rcar-vin: use different v4l2 operations in media controller mode")
> Reported-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 2fb8587116f25a4f..e78fba84d59028ef 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -702,6 +702,12 @@ static int rvin_mc_s_fmt_vid_cap(struct file *file, void *priv,
>  
>  	vin->format = f->fmt.pix;
>  
> +	vin->crop.top = 0;
> +	vin->crop.left = 0;
> +	vin->crop.width = vin->format.width;
> +	vin->crop.height = vin->format.height;
> +	vin->compose = vin->crop;
> +
>  	return 0;
>  }
>  
> 

Re: [PATCH v2 1/2] Revert "media: rcar-vin: enable field toggle after a set number of lines for Gen3"

[Kieran Bingham]

Hi Niklas,

On 11/05/18 15:41, Niklas Söderlund wrote:
> The offending commit was an attempt to fix the issue of writing outside
> the capture buffer for VIN Gen3. Unfortunately it only fixed the symptom
> of the problem to such a degree I could no longer reproduce it. Revert
> the offending commit before a proper fix can be added in a follow-up
> patch.
> 
> This reverts commit 015060cb7795eac34454696cc9c9f8b76926a401.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Acked-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index b41ba9a4a2b3ac90..ac07f99e3516a620 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -124,9 +124,7 @@
>  #define VNDMR2_VPS		(1 << 30)
>  #define VNDMR2_HPS		(1 << 29)
>  #define VNDMR2_FTEV		(1 << 17)
> -#define VNDMR2_FTEH		(1 << 16)
>  #define VNDMR2_VLV(n)		((n & 0xf) << 12)
> -#define VNDMR2_HLV(n)		((n) & 0xfff)
>  
>  /* Video n CSI2 Interface Mode Register (Gen3) */
>  #define VNCSI_IFMD_DES1		(1 << 26)
> @@ -614,9 +612,8 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>  
>  static int rvin_setup(struct rvin_dev *vin)
>  {
> -	u32 vnmc, dmr, dmr2, interrupts, lines;
> +	u32 vnmc, dmr, dmr2, interrupts;
>  	bool progressive = false, output_is_yuv = false, input_is_yuv = false;
> -	bool halfsize = false;
>  
>  	switch (vin->format.field) {
>  	case V4L2_FIELD_TOP:
> @@ -631,15 +628,12 @@ static int rvin_setup(struct rvin_dev *vin)
>  		/* Use BT if video standard can be read and is 60 Hz format */
>  		if (!vin->info->use_mc && vin->std & V4L2_STD_525_60)
>  			vnmc = VNMC_IM_FULL | VNMC_FOC;
> -		halfsize = true;
>  		break;
>  	case V4L2_FIELD_INTERLACED_TB:
>  		vnmc = VNMC_IM_FULL;
> -		halfsize = true;
>  		break;
>  	case V4L2_FIELD_INTERLACED_BT:
>  		vnmc = VNMC_IM_FULL | VNMC_FOC;
> -		halfsize = true;
>  		break;
>  	case V4L2_FIELD_NONE:
>  		vnmc = VNMC_IM_ODD_EVEN;
> @@ -682,15 +676,11 @@ static int rvin_setup(struct rvin_dev *vin)
>  		break;
>  	}
>  
> -	if (vin->info->model == RCAR_GEN3) {
> -		/* Enable HSYNC Field Toggle mode after height HSYNC inputs. */
> -		lines = vin->format.height / (halfsize ? 2 : 1);
> -		dmr2 = VNDMR2_FTEH | VNDMR2_HLV(lines);
> -		vin_dbg(vin, "Field Toogle after %u lines\n", lines);
> -	} else {
> -		/* Enable VSYNC Field Toogle mode after one VSYNC input. */
> +	/* Enable VSYNC Field Toogle mode after one VSYNC input */
> +	if (vin->info->model == RCAR_GEN3)
> +		dmr2 = VNDMR2_FTEV;
> +	else
>  		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
> -	}
>  
>  	/* Hsync Signal Polarity Select */
>  	if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
>