| Message ID | 1439851884-3346-2-git-send-email-tjakobi@math.uni-bielefeld.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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)
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)
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@math.uni-bielefeld.de> --- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 51 ++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 10 deletions(-)