Re: [PATCH 2/4] drm/displayid: Iterate over all DisplayID blocks

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 10 May 2016, Dave Airlie <airlied@gmail.com> wrote:
> From: Tomas Bzatek <tomas@bzatek.net>
>
> This will iterate over all DisplayID blocks found in the buffer.
> Previously only the first block was parsed.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=95207
>
> Signed-off-by: Tomas Bzatek <tomas@bzatek.net>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 9dbaaf4..3cf17a3 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4237,18 +4237,24 @@ static int drm_parse_display_id(struct drm_connector *connector,
>  		return -EINVAL;
>  	}
>  
> -	block = (struct displayid_block *)&displayid[idx + 4];
> -	DRM_DEBUG_KMS("block id %d, rev %d, len %d\n",
> -		      block->tag, block->rev, block->num_bytes);
> -
> -	switch (block->tag) {
> -	case DATA_BLOCK_TILED_DISPLAY:
> -		ret = drm_parse_tiled_block(connector, block);
> -		if (ret)
> -			return ret;
> -	default:
> -		printk("unknown displayid tag %d\n", block->tag);
> -		break;
> +	idx += sizeof(struct displayid_hdr);
> +	while (block = (struct displayid_block *)&displayid[idx],
> +	       idx + sizeof(struct displayid_block) <= length &&
> +	       idx + sizeof(struct displayid_block) + block->num_bytes <= length &&
> +	       block->num_bytes > 0) {

Ugh, that's a bit ugly, but seems to DTRT.

Do we need to check block->num_bytes > 0? Shouldn't that be block->tag
specific? See below.

> +		idx += block->num_bytes + sizeof(struct displayid_block);
> +		DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n",
> +			      block->tag, block->rev, block->num_bytes);
> +
> +		switch (block->tag) {
> +		case DATA_BLOCK_TILED_DISPLAY:

This could use a check (in a separate patch, perhaps) to verify idx +
sizeof(struct displayid_tiled_block) <= length. (Alternatively,
sizeof(struct displayid_block) + block->num_bytes == sizeof(struct
displayid_tiled_block).)

In any case,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +			ret = drm_parse_tiled_block(connector, block);
> +			if (ret)
> +				return ret;
> +		default:
> +			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
> +			break;
> +		}
>  	}
>  	return 0;
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel