diff mbox series

media: vicodec: ensure comp frame pointer kept in range

Message ID 20190122150034.49696-1-dafna3@gmail.com (mailing list archive)
State New, archived
Headers show
Series media: vicodec: ensure comp frame pointer kept in range | expand

Commit Message

Dafna Hirschfeld Jan. 22, 2019, 3 p.m. UTC
Make sure that the pointer to the compressed frame does not
get out of the buffer.

Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
---
 drivers/media/platform/vicodec/codec-fwht.c   | 73 +++++++++++++------
 drivers/media/platform/vicodec/codec-fwht.h   |  8 +-
 .../media/platform/vicodec/codec-v4l2-fwht.c  |  8 +-
 drivers/media/platform/vicodec/vicodec-core.c |  4 +
 4 files changed, 62 insertions(+), 31 deletions(-)

Comments

Hans Verkuil Jan. 23, 2019, 9:51 a.m. UTC | #1
Hi Dafna,

On 01/22/19 16:00, Dafna Hirschfeld wrote:
> Make sure that the pointer to the compressed frame does not
> get out of the buffer.
> 
> Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
> ---
>  drivers/media/platform/vicodec/codec-fwht.c   | 73 +++++++++++++------
>  drivers/media/platform/vicodec/codec-fwht.h   |  8 +-
>  .../media/platform/vicodec/codec-v4l2-fwht.c  |  8 +-
>  drivers/media/platform/vicodec/vicodec-core.c |  4 +
>  4 files changed, 62 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c
> index e5e0a80c2f73..e12f04a99d3f 100644
> --- a/drivers/media/platform/vicodec/codec-fwht.c
> +++ b/drivers/media/platform/vicodec/codec-fwht.c
> @@ -104,16 +104,20 @@ static int rlc(const s16 *in, __be16 *output, int blocktype)
>   * This function will worst-case increase rlc_in by 65*2 bytes:
>   * one s16 value for the header and 8 * 8 coefficients of type s16.
>   */
> -static s16 derlc(const __be16 **rlc_in, s16 *dwht_out)
> +static int derlc(const __be16 **rlc_in, s16 *dwht_out, s16 *stat,
> +		 const __be16 *after_rlco)

Hmm. I think this is confusing.

I would add a

#define OVERFLOW_BIT BIT(14)

and return that when a buffer overrun is detected.

I'm not sure why derlc returned a s16 instead of a u16, so let's fix that
as well.

The name 'after_rlco' is, I think, very confusing. How about 'end_of_buffer'
which the last valid address of the buffer (so one less than after_rlco).

>  {
>  	/* header */
>  	const __be16 *input = *rlc_in;
> -	s16 ret = ntohs(*input++);
>  	int dec_count = 0;
>  	s16 block[8 * 8 + 16];
>  	s16 *wp = block;
>  	int i;
>  
> +	if (input >= after_rlco)
> +		return -1;
> +	*stat = ntohs(*input++);
> +
>  	/*
>  	 * Now de-compress, it expands one byte to up to 15 bytes
>  	 * (or fills the remainder of the 64 bytes with zeroes if it
> @@ -123,9 +127,15 @@ static s16 derlc(const __be16 **rlc_in, s16 *dwht_out)
>  	 * allow for overflow if the incoming data was malformed.
>  	 */
>  	while (dec_count < 8 * 8) {
> -		s16 in = ntohs(*input++);
> -		int length = in & 0xf;
> -		int coeff = in >> 4;
> +		s16 in;
> +		int length;
> +		int coeff;
> +
> +		if (input >= after_rlco)
> +			return -1;
> +		in = ntohs(*input++);
> +		length = in & 0xf;
> +		coeff = in >> 4;
>  
>  		/* fill remainder with zeros */
>  		if (length == 15) {
> @@ -150,7 +160,7 @@ static s16 derlc(const __be16 **rlc_in, s16 *dwht_out)
>  		dwht_out[x + y * 8] = *wp++;
>  	}
>  	*rlc_in = input;
> -	return ret;
> +	return 0;
>  }
>  
>  static const int quant_table[] = {
> @@ -808,9 +818,9 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
>  	return encoding;
>  }
>  
> -static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
> -			 u32 height, u32 width, u32 coded_width,
> -			 bool uncompressed)
> +static int decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
> +			u32 height, u32 width, u32 coded_width,
> +			bool uncompressed, const __be16 *after_rlco)

I think returning a bool here makes more sense: true == success, false == failure.

>  {
>  	unsigned int copies = 0;
>  	s16 copy[8 * 8];
> @@ -821,9 +831,11 @@ static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
>  	height = round_up(height, 8);
>  
>  	if (uncompressed) {
> +		if (after_rlco < *rlco + width * height / 2)
> +			return -1;
>  		memcpy(ref, *rlco, width * height);
>  		*rlco += width * height / 2;
> -		return;
> +		return 0;
>  	}
>  
>  	/*
> @@ -835,6 +847,7 @@ static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
>  	for (j = 0; j < height / 8; j++) {
>  		for (i = 0; i < width / 8; i++) {
>  			u8 *refp = ref + j * 8 * coded_width + i * 8;
> +			int ret;
>  
>  			if (copies) {
>  				memcpy(cf->de_fwht, copy, sizeof(copy));
> @@ -847,8 +860,9 @@ static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
>  				continue;
>  			}
>  
> -			stat = derlc(rlco, cf->coeffs);
> -
> +			ret = derlc(rlco, cf->coeffs, &stat, after_rlco);
> +			if (ret < 0)
> +				return -1;
>  			if (stat & PFRAME_BIT)
>  				dequantize_inter(cf->coeffs);
>  			else
> @@ -865,17 +879,21 @@ static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
>  			fill_decoder_block(refp, cf->de_fwht, coded_width);
>  		}
>  	}
> +	return 0;
>  }
>  
> -void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
> -		       u32 hdr_flags, unsigned int components_num,
> -		       unsigned int width, unsigned int height,
> -		       unsigned int coded_width)
> +int fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,

Also bool.

> +		      u32 hdr_flags, unsigned int components_num,
> +		      unsigned int width, unsigned int height,
> +		      unsigned int coded_width)
>  {
>  	const __be16 *rlco = cf->rlc_data;
> +	const __be16 *after_rlco = cf->rlc_data + (cf->size / sizeof(*rlco));
>  
> -	decode_plane(cf, &rlco, ref->luma, height, width, coded_width,
> -		     hdr_flags & FWHT_FL_LUMA_IS_UNCOMPRESSED);
> +	if (decode_plane(cf, &rlco, ref->luma, height, width, coded_width,
> +			 hdr_flags & FWHT_FL_LUMA_IS_UNCOMPRESSED,
> +			 after_rlco) < 0)
> +		return -1;
>  
>  	if (components_num >= 3) {
>  		u32 h = height;
> @@ -888,13 +906,20 @@ void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
>  			w /= 2;
>  			c /= 2;
>  		}
> -		decode_plane(cf, &rlco, ref->cb, h, w, c,
> -			     hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED);
> -		decode_plane(cf, &rlco, ref->cr, h, w, c,
> -			     hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED);
> +		if (decode_plane(cf, &rlco, ref->cb, h, w, c,
> +				 hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED,
> +				 after_rlco) < 0)
> +			return -1;
> +		if (decode_plane(cf, &rlco, ref->cr, h, w, c,
> +				 hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED,
> +				 after_rlco) < 0)
> +			return -1;
>  	}
>  
>  	if (components_num == 4)
> -		decode_plane(cf, &rlco, ref->alpha, height, width, coded_width,
> -			     hdr_flags & FWHT_FL_ALPHA_IS_UNCOMPRESSED);
> +		if (decode_plane(cf, &rlco, ref->alpha, height, width, coded_width,
> +				 hdr_flags & FWHT_FL_ALPHA_IS_UNCOMPRESSED,
> +				 after_rlco) < 0)
> +			return -1;
> +	return 0;
>  }
> diff --git a/drivers/media/platform/vicodec/codec-fwht.h b/drivers/media/platform/vicodec/codec-fwht.h
> index ad8cfc60a152..ce7aa8a4d761 100644
> --- a/drivers/media/platform/vicodec/codec-fwht.h
> +++ b/drivers/media/platform/vicodec/codec-fwht.h
> @@ -139,9 +139,9 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm,
>  		      bool is_intra, bool next_is_intra,
>  		      unsigned int width, unsigned int height,
>  		      unsigned int stride, unsigned int chroma_stride);
> -void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
> -		       u32 hdr_flags, unsigned int components_num,
> -		       unsigned int width, unsigned int height,
> -		       unsigned int coded_width);
> +int fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
> +		      u32 hdr_flags, unsigned int components_num,
> +		      unsigned int width, unsigned int height,
> +		      unsigned int coded_width);
>  
>  #endif
> diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
> index ee6903b8896c..d8c58d0a667c 100644
> --- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c
> +++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
> @@ -280,6 +280,7 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
>  	state->ycbcr_enc = ntohl(state->header.ycbcr_enc);
>  	state->quantization = ntohl(state->header.quantization);
>  	cf.rlc_data = (__be16 *)p_in;
> +	cf.size = ntohl(state->header.size);
>  
>  	hdr_width_div = (flags & FWHT_FL_CHROMA_FULL_WIDTH) ? 1 : 2;
>  	hdr_height_div = (flags & FWHT_FL_CHROMA_FULL_HEIGHT) ? 1 : 2;
> @@ -287,9 +288,10 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
>  	    hdr_height_div != info->height_div)
>  		return -EINVAL;
>  
> -	fwht_decode_frame(&cf, &state->ref_frame, flags, components_num,
> -			  state->visible_width, state->visible_height,
> -			  state->coded_width);
> +	if (fwht_decode_frame(&cf, &state->ref_frame, flags, components_num,
> +			      state->visible_width, state->visible_height,
> +			      state->coded_width) < 0)
> +		return -EINVAL;
>  
>  	/*
>  	 * TODO - handle the case where the compressed stream encodes a
> diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
> index b84dae343645..953b9c4816a5 100644
> --- a/drivers/media/platform/vicodec/vicodec-core.c
> +++ b/drivers/media/platform/vicodec/vicodec-core.c
> @@ -186,6 +186,10 @@ static int device_process(struct vicodec_ctx *ctx,
>  			return ret;
>  		vb2_set_plane_payload(&dst_vb->vb2_buf, 0, ret);
>  	} else {
> +		unsigned int comp_frame_size = ntohl(ctx->state.header.size);
> +
> +		if (comp_frame_size > ctx->comp_max_size)
> +			return -EINVAL;
>  		state->info = q_dst->info;
>  		ret = v4l2_fwht_decode(state, p_src, p_dst);
>  		if (ret < 0)
> 

Regards,

	Hans
diff mbox series

Patch

diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c
index e5e0a80c2f73..e12f04a99d3f 100644
--- a/drivers/media/platform/vicodec/codec-fwht.c
+++ b/drivers/media/platform/vicodec/codec-fwht.c
@@ -104,16 +104,20 @@  static int rlc(const s16 *in, __be16 *output, int blocktype)
  * This function will worst-case increase rlc_in by 65*2 bytes:
  * one s16 value for the header and 8 * 8 coefficients of type s16.
  */
-static s16 derlc(const __be16 **rlc_in, s16 *dwht_out)
+static int derlc(const __be16 **rlc_in, s16 *dwht_out, s16 *stat,
+		 const __be16 *after_rlco)
 {
 	/* header */
 	const __be16 *input = *rlc_in;
-	s16 ret = ntohs(*input++);
 	int dec_count = 0;
 	s16 block[8 * 8 + 16];
 	s16 *wp = block;
 	int i;
 
+	if (input >= after_rlco)
+		return -1;
+	*stat = ntohs(*input++);
+
 	/*
 	 * Now de-compress, it expands one byte to up to 15 bytes
 	 * (or fills the remainder of the 64 bytes with zeroes if it
@@ -123,9 +127,15 @@  static s16 derlc(const __be16 **rlc_in, s16 *dwht_out)
 	 * allow for overflow if the incoming data was malformed.
 	 */
 	while (dec_count < 8 * 8) {
-		s16 in = ntohs(*input++);
-		int length = in & 0xf;
-		int coeff = in >> 4;
+		s16 in;
+		int length;
+		int coeff;
+
+		if (input >= after_rlco)
+			return -1;
+		in = ntohs(*input++);
+		length = in & 0xf;
+		coeff = in >> 4;
 
 		/* fill remainder with zeros */
 		if (length == 15) {
@@ -150,7 +160,7 @@  static s16 derlc(const __be16 **rlc_in, s16 *dwht_out)
 		dwht_out[x + y * 8] = *wp++;
 	}
 	*rlc_in = input;
-	return ret;
+	return 0;
 }
 
 static const int quant_table[] = {
@@ -808,9 +818,9 @@  u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 	return encoding;
 }
 
-static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
-			 u32 height, u32 width, u32 coded_width,
-			 bool uncompressed)
+static int decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
+			u32 height, u32 width, u32 coded_width,
+			bool uncompressed, const __be16 *after_rlco)
 {
 	unsigned int copies = 0;
 	s16 copy[8 * 8];
@@ -821,9 +831,11 @@  static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
 	height = round_up(height, 8);
 
 	if (uncompressed) {
+		if (after_rlco < *rlco + width * height / 2)
+			return -1;
 		memcpy(ref, *rlco, width * height);
 		*rlco += width * height / 2;
-		return;
+		return 0;
 	}
 
 	/*
@@ -835,6 +847,7 @@  static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
 	for (j = 0; j < height / 8; j++) {
 		for (i = 0; i < width / 8; i++) {
 			u8 *refp = ref + j * 8 * coded_width + i * 8;
+			int ret;
 
 			if (copies) {
 				memcpy(cf->de_fwht, copy, sizeof(copy));
@@ -847,8 +860,9 @@  static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
 				continue;
 			}
 
-			stat = derlc(rlco, cf->coeffs);
-
+			ret = derlc(rlco, cf->coeffs, &stat, after_rlco);
+			if (ret < 0)
+				return -1;
 			if (stat & PFRAME_BIT)
 				dequantize_inter(cf->coeffs);
 			else
@@ -865,17 +879,21 @@  static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref,
 			fill_decoder_block(refp, cf->de_fwht, coded_width);
 		}
 	}
+	return 0;
 }
 
-void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
-		       u32 hdr_flags, unsigned int components_num,
-		       unsigned int width, unsigned int height,
-		       unsigned int coded_width)
+int fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
+		      u32 hdr_flags, unsigned int components_num,
+		      unsigned int width, unsigned int height,
+		      unsigned int coded_width)
 {
 	const __be16 *rlco = cf->rlc_data;
+	const __be16 *after_rlco = cf->rlc_data + (cf->size / sizeof(*rlco));
 
-	decode_plane(cf, &rlco, ref->luma, height, width, coded_width,
-		     hdr_flags & FWHT_FL_LUMA_IS_UNCOMPRESSED);
+	if (decode_plane(cf, &rlco, ref->luma, height, width, coded_width,
+			 hdr_flags & FWHT_FL_LUMA_IS_UNCOMPRESSED,
+			 after_rlco) < 0)
+		return -1;
 
 	if (components_num >= 3) {
 		u32 h = height;
@@ -888,13 +906,20 @@  void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
 			w /= 2;
 			c /= 2;
 		}
-		decode_plane(cf, &rlco, ref->cb, h, w, c,
-			     hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED);
-		decode_plane(cf, &rlco, ref->cr, h, w, c,
-			     hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED);
+		if (decode_plane(cf, &rlco, ref->cb, h, w, c,
+				 hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED,
+				 after_rlco) < 0)
+			return -1;
+		if (decode_plane(cf, &rlco, ref->cr, h, w, c,
+				 hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED,
+				 after_rlco) < 0)
+			return -1;
 	}
 
 	if (components_num == 4)
-		decode_plane(cf, &rlco, ref->alpha, height, width, coded_width,
-			     hdr_flags & FWHT_FL_ALPHA_IS_UNCOMPRESSED);
+		if (decode_plane(cf, &rlco, ref->alpha, height, width, coded_width,
+				 hdr_flags & FWHT_FL_ALPHA_IS_UNCOMPRESSED,
+				 after_rlco) < 0)
+			return -1;
+	return 0;
 }
diff --git a/drivers/media/platform/vicodec/codec-fwht.h b/drivers/media/platform/vicodec/codec-fwht.h
index ad8cfc60a152..ce7aa8a4d761 100644
--- a/drivers/media/platform/vicodec/codec-fwht.h
+++ b/drivers/media/platform/vicodec/codec-fwht.h
@@ -139,9 +139,9 @@  u32 fwht_encode_frame(struct fwht_raw_frame *frm,
 		      bool is_intra, bool next_is_intra,
 		      unsigned int width, unsigned int height,
 		      unsigned int stride, unsigned int chroma_stride);
-void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
-		       u32 hdr_flags, unsigned int components_num,
-		       unsigned int width, unsigned int height,
-		       unsigned int coded_width);
+int fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref,
+		      u32 hdr_flags, unsigned int components_num,
+		      unsigned int width, unsigned int height,
+		      unsigned int coded_width);
 
 #endif
diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
index ee6903b8896c..d8c58d0a667c 100644
--- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c
+++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
@@ -280,6 +280,7 @@  int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	state->ycbcr_enc = ntohl(state->header.ycbcr_enc);
 	state->quantization = ntohl(state->header.quantization);
 	cf.rlc_data = (__be16 *)p_in;
+	cf.size = ntohl(state->header.size);
 
 	hdr_width_div = (flags & FWHT_FL_CHROMA_FULL_WIDTH) ? 1 : 2;
 	hdr_height_div = (flags & FWHT_FL_CHROMA_FULL_HEIGHT) ? 1 : 2;
@@ -287,9 +288,10 @@  int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 	    hdr_height_div != info->height_div)
 		return -EINVAL;
 
-	fwht_decode_frame(&cf, &state->ref_frame, flags, components_num,
-			  state->visible_width, state->visible_height,
-			  state->coded_width);
+	if (fwht_decode_frame(&cf, &state->ref_frame, flags, components_num,
+			      state->visible_width, state->visible_height,
+			      state->coded_width) < 0)
+		return -EINVAL;
 
 	/*
 	 * TODO - handle the case where the compressed stream encodes a
diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index b84dae343645..953b9c4816a5 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -186,6 +186,10 @@  static int device_process(struct vicodec_ctx *ctx,
 			return ret;
 		vb2_set_plane_payload(&dst_vb->vb2_buf, 0, ret);
 	} else {
+		unsigned int comp_frame_size = ntohl(ctx->state.header.size);
+
+		if (comp_frame_size > ctx->comp_max_size)
+			return -EINVAL;
 		state->info = q_dst->info;
 		ret = v4l2_fwht_decode(state, p_src, p_dst);
 		if (ret < 0)