drm/exynos: fix size check in g2d_check_buf_desc_is_valid()

[Tobias Jakobi]

Gentle ping!

Also please note that this is a critical fix. With the
incomplete check pagefaults can happen when the engine
accesses a invalid buffer position.

With best wishes,
Tobias


On 2015-08-18 00:51, Tobias Jakobi wrote:
> The size check was incomplete. It only computed the
> size of area of the drawing rectangle and checked if
> the size still fit inside the buffer.
> 
> The correct check is to compute the position of the
> last byte that the G2D engine is going to access and
> then check if that position is still contained in the
> buffer. In particular we need the stride information
> to determine this.
> 
> Signed-off-by: Tobias Jakobi <tjakobi at math.uni-bielefeld.de>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_g2d.c | 51 
> ++++++++++++++++++++++++++-------
>  1 file changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> index 211af37..535b4ad 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> @@ -48,11 +48,13 @@
> 
>  /* registers for base address */
>  #define G2D_SRC_BASE_ADDR		0x0304
> +#define G2D_SRC_STRIDE_REG		0x0308
>  #define G2D_SRC_COLOR_MODE		0x030C
>  #define G2D_SRC_LEFT_TOP		0x0310
>  #define G2D_SRC_RIGHT_BOTTOM		0x0314
>  #define G2D_SRC_PLANE2_BASE_ADDR	0x0318
>  #define G2D_DST_BASE_ADDR		0x0404
> +#define G2D_DST_STRIDE_REG		0x0408
>  #define G2D_DST_COLOR_MODE		0x040C
>  #define G2D_DST_LEFT_TOP		0x0410
>  #define G2D_DST_RIGHT_BOTTOM		0x0414
> @@ -148,6 +150,7 @@ struct g2d_cmdlist {
>   * A structure of buffer description
>   *
>   * @format: color format
> + * @stride: buffer stride/pitch in bytes
>   * @left_x: the x coordinates of left top corner
>   * @top_y: the y coordinates of left top corner
>   * @right_x: the x coordinates of right bottom corner
> @@ -156,6 +159,7 @@ struct g2d_cmdlist {
>   */
>  struct g2d_buf_desc {
>  	unsigned int	format;
> +	unsigned int	stride;
>  	unsigned int	left_x;
>  	unsigned int	top_y;
>  	unsigned int	right_x;
> @@ -589,6 +593,7 @@ static enum g2d_reg_type g2d_get_reg_type(int 
> reg_offset)
> 
>  	switch (reg_offset) {
>  	case G2D_SRC_BASE_ADDR:
> +	case G2D_SRC_STRIDE_REG:
>  	case G2D_SRC_COLOR_MODE:
>  	case G2D_SRC_LEFT_TOP:
>  	case G2D_SRC_RIGHT_BOTTOM:
> @@ -598,6 +603,7 @@ static enum g2d_reg_type g2d_get_reg_type(int 
> reg_offset)
>  		reg_type = REG_TYPE_SRC_PLANE2;
>  		break;
>  	case G2D_DST_BASE_ADDR:
> +	case G2D_DST_STRIDE_REG:
>  	case G2D_DST_COLOR_MODE:
>  	case G2D_DST_LEFT_TOP:
>  	case G2D_DST_RIGHT_BOTTOM:
> @@ -652,8 +658,8 @@ static bool g2d_check_buf_desc_is_valid(struct
> g2d_buf_desc *buf_desc,
>  						enum g2d_reg_type reg_type,
>  						unsigned long size)
>  {
> -	unsigned int width, height;
> -	unsigned long area;
> +	int width, height;
> +	unsigned long bpp, last_pos;
> 
>  	/*
>  	 * check source and destination buffers only.
> @@ -662,22 +668,37 @@ static bool g2d_check_buf_desc_is_valid(struct
> g2d_buf_desc *buf_desc,
>  	if (reg_type != REG_TYPE_SRC && reg_type != REG_TYPE_DST)
>  		return true;
> 
> -	width = buf_desc->right_x - buf_desc->left_x;
> +	/* This check also makes sure that right_x > left_x. */
> +	width = (int)buf_desc->right_x - (int)buf_desc->left_x;
>  	if (width < G2D_LEN_MIN || width > G2D_LEN_MAX) {
> -		DRM_ERROR("width[%u] is out of range!\n", width);
> +		DRM_ERROR("width[%d] is out of range!\n", width);
>  		return false;
>  	}
> 
> -	height = buf_desc->bottom_y - buf_desc->top_y;
> +	/* This check also makes sure that bottom_y > top_y. */
> +	height = (int)buf_desc->bottom_y - (int)buf_desc->top_y;
>  	if (height < G2D_LEN_MIN || height > G2D_LEN_MAX) {
> -		DRM_ERROR("height[%u] is out of range!\n", height);
> +		DRM_ERROR("height[%d] is out of range!\n", height);
>  		return false;
>  	}
> 
> -	area = (unsigned long)width * (unsigned long)height *
> -					g2d_get_buf_bpp(buf_desc->format);
> -	if (area > size) {
> -		DRM_ERROR("area[%lu] is out of range[%lu]!\n", area, size);
> +	bpp = g2d_get_buf_bpp(buf_desc->format);
> +
> +	/* Compute the position of the last byte that the engine accesses. */
> +	last_pos = ((unsigned long)buf_desc->bottom_y - 1) *
> +		(unsigned long)buf_desc->stride +
> +		(unsigned long)buf_desc->right_x * bpp - 1;
> +
> +	/*
> +	 * Since right_x > left_x and bottom_y > top_y we already know
> +	 * that the first_pos < last_pos (first_pos being the position
> +	 * of the first byte the engine accesses), it just remains to
> +	 * check if last_pos is smaller then the buffer size.
> +	 */
> +
> +	if (last_pos >= size) {
> +		DRM_ERROR("last engine access position [%lu] "
> +			"is out of range [%lu]!\n", last_pos, size);
>  		return false;
>  	}
> 
> @@ -981,6 +1002,16 @@ static int g2d_check_reg_offset(struct device 
> *dev,
>  			} else
>  				buf_info->types[reg_type] = BUF_TYPE_GEM;
>  			break;
> +		case G2D_SRC_STRIDE_REG:
> +		case G2D_DST_STRIDE_REG:
> +			if (for_addr)
> +				goto err;
> +
> +			reg_type = g2d_get_reg_type(reg_offset);
> +
> +			buf_desc = &buf_info->descs[reg_type];
> +			buf_desc->stride = cmdlist->data[index + 1];
> +			break;
>  		case G2D_SRC_COLOR_MODE:
>  		case G2D_DST_COLOR_MODE:
>  			if (for_addr)