Re: [PATCH 1/2] media: coda: jpeg: support optimized huffman tables

[Philipp Zabel <pza@pengutronix.de>]

Hi Adrian,

On Mon, Mar 23, 2020 at 03:09:36PM +0200, Adrian Ratiu wrote:
> Each jpeg can have the huffman tables optimized for its specific content
> meaning that the table lenghts and values don't match the standard table
> of substitutions so there's no reason to hardcode and expect the sandard
> lenghts, otherwise we just end up rejecting optimized jpegs altogether.

Thank you, that is a great improvement. There's one issue remaining
below:

> Tested on CODA960.
> 
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  drivers/media/platform/coda/coda-jpeg.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/coda/coda-jpeg.c b/drivers/media/platform/coda/coda-jpeg.c
> index 6a11b64efb6b..162ba28a6b95 100644
> --- a/drivers/media/platform/coda/coda-jpeg.c
> +++ b/drivers/media/platform/coda/coda-jpeg.c
> @@ -343,7 +343,8 @@ int coda_jpeg_decode_header(struct coda_ctx *ctx, struct vb2_buffer *vb)
>  			v4l2_err(&dev->v4l2_dev, "missing Huffman table\n");
>  			return -EINVAL;
>  		}
> -		if (huffman_tables[i].length != ((i & 2) ? 178 : 28)) {
> +		if (huffman_tables[i].length < 17 ||
> +		    huffman_tables[i].length > 178) {

The maximum length of the DC tables is 16 + 12, so this should
still be checked for a maximum length of 28 if (i & 2) == 0.

>  			v4l2_err(&dev->v4l2_dev,
>  				 "invalid Huffman table %d length: %zu\n",
>  				 i, huffman_tables[i].length);
> @@ -357,10 +358,12 @@ int coda_jpeg_decode_header(struct coda_ctx *ctx, struct vb2_buffer *vb)
>  			return -ENOMEM;
>  		ctx->params.jpeg_huff_tab = huff_tab;
>  	}
> -	memcpy(huff_tab->luma_dc, huffman_tables[0].start, 16 + 12);
> -	memcpy(huff_tab->chroma_dc, huffman_tables[1].start, 16 + 12);
> -	memcpy(huff_tab->luma_ac, huffman_tables[2].start, 16 + 162);
> -	memcpy(huff_tab->chroma_ac, huffman_tables[3].start, 16 + 162);
> +
> +	memset(huff_tab, 0, sizeof(*huff_tab));
> +	memcpy(huff_tab->luma_dc, huffman_tables[0].start, huffman_tables[0].length);
> +	memcpy(huff_tab->chroma_dc, huffman_tables[1].start, huffman_tables[1].length);

Otherwise these two might overflow the luma_dc and chroma_dc arrays.

regards
Philipp