Re: [PATCH v2 1/8] v4l: vsp1: Protect fragments against overflow

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

Thank you for the patch.

On Monday 14 Aug 2017 16:13:24 Kieran Bingham wrote:
> The fragment write function relies on the code never asking it to
> write more than the entries available in the list.
> 
> Currently with each list body containing 256 entries, this is fine,
> but we can reduce this number greatly saving memory.
> 
> In preparation of this - add a level of protection to catch any
> buffer overflows.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_dl.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index 8b5cbb6b7a70..cb4625ae13c2
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -50,6 +50,7 @@ struct vsp1_dl_entry {
>   * @dma: DMA address of the entries
>   * @size: size of the DMA memory in bytes
>   * @num_entries: number of stored entries
> + * @max_entries: number of entries available
>   */
>  struct vsp1_dl_body {
>  	struct list_head list;
> @@ -60,6 +61,7 @@ struct vsp1_dl_body {
>  	size_t size;
> 
>  	unsigned int num_entries;
> +	unsigned int max_entries;
>  };
> 
>  /**
> @@ -138,6 +140,7 @@ static int vsp1_dl_body_init(struct vsp1_device *vsp1,
> 
>  	dlb->vsp1 = vsp1;
>  	dlb->size = size;
> +	dlb->max_entries = num_entries;
> 
>  	dlb->entries = dma_alloc_wc(vsp1->bus_master, dlb->size, &dlb->dma,
>  				    GFP_KERNEL);
> @@ -220,6 +223,11 @@ void vsp1_dl_fragment_free(struct vsp1_dl_body *dlb)
>   */
>  void vsp1_dl_fragment_write(struct vsp1_dl_body *dlb, u32 reg, u32 data)
>  {
> +	if (unlikely(dlb->num_entries >= dlb->max_entries)) {
> +		WARN_ONCE(true, "DLB size exceeded (max %u)", dlb-
>max_entries);
> +		return;
> +	}

How about

	if (WARN_ONCE(dlb->num_entries >= dlb->max_entries,
		      "DLB size exceeded (max %u)", dlb->max_entries))
		return;

(WARN_ONCE contains the unlikely() already)

I'm not fussed either way,

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

>  	dlb->entries[dlb->num_entries].addr = reg;
>  	dlb->entries[dlb->num_entries].data = data;
>  	dlb->num_entries++;

-- 
Regards,

Laurent Pinchart