* [PATCH] media: vicodec: ensure comp frame pointer kept in range
@ 2019-01-22 15:00 Dafna Hirschfeld
2019-01-23 9:51 ` Hans Verkuil
0 siblings, 1 reply; 2+ messages in thread
From: Dafna Hirschfeld @ 2019-01-22 15:00 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, helen.koike, Dafna Hirschfeld
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)
{
/* 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)
--
2.17.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] media: vicodec: ensure comp frame pointer kept in range
2019-01-22 15:00 [PATCH] media: vicodec: ensure comp frame pointer kept in range Dafna Hirschfeld
@ 2019-01-23 9:51 ` Hans Verkuil
0 siblings, 0 replies; 2+ messages in thread
From: Hans Verkuil @ 2019-01-23 9:51 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media; +Cc: helen.koike
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-01-23 9:51 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-22 15:00 [PATCH] media: vicodec: ensure comp frame pointer kept in range Dafna Hirschfeld
2019-01-23 9:51 ` Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).