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@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
> dlb->entries[dlb->num_entries].addr = reg;
> dlb->entries[dlb->num_entries].data = data;
> dlb->num_entries++;
--
Regards,
Laurent Pinchart
