diff mbox series

[FFmpeg-devel,v1] avcodec/cbs_vp8: Improve the bitstream position check

Message ID DS7PR11MB7949CF2C01F31B4B8597EC61B17A2@DS7PR11MB7949.namprd11.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,v1] avcodec/cbs_vp8: Improve the bitstream position check | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Dai, Jianhui J Jan. 25, 2024, 12:54 a.m. UTC 
The VP8 compressed header may not be byte-aligned due to boolean
coding. Use bitwise comparison to prevent the potential overread.

Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com>
---
 libavcodec/cbs_vp8.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Dai, Jianhui J Feb. 23, 2024, 12:43 a.m. UTC | #1 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Dai,
> Jianhui J
> Sent: Thursday, January 25, 2024 8:54 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH v1] avcodec/cbs_vp8: Improve the bitstream
> position check
> 
> The VP8 compressed header may not be byte-aligned due to boolean coding. Use
> bitwise comparison to prevent the potential overread.
> 
> Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com>
> ---
>  libavcodec/cbs_vp8.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c index
> 065156c248..13acad3724 100644
> --- a/libavcodec/cbs_vp8.c
> +++ b/libavcodec/cbs_vp8.c
> @@ -327,9 +327,10 @@ static int cbs_vp8_read_unit(CodedBitstreamContext
> *ctx,
>      if (err < 0)
>          return err;
> 
> +    // Position may not be byte-aligned after compressed header; using bits
> +    // count comparison for accuracy.
>      pos = get_bits_count(&gbc);
> -    pos /= 8;
> -    av_assert0(pos <= unit->data_size);
> +    av_assert0(pos <= unit->data_size * 8);
> 
>      frame->data_ref = av_buffer_ref(unit->data_ref);
>      if (!frame->data_ref)

Ping reviewers to help to apply. 

The review history can be found here:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/CH3PR11MB793797554CDB411074364733B1742@CH3PR11MB7937.namprd11.prod.outlook.com/

> --
> 2.25.1
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with
> subject "unsubscribe".
Dai, Jianhui J March 5, 2024, 5:33 a.m. UTC | #2 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Dai,
> Jianhui J
> Sent: Friday, February 23, 2024 8:43 AM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v1] avcodec/cbs_vp8: Improve the
> bitstream position check
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Dai,
> > Jianhui J
> > Sent: Thursday, January 25, 2024 8:54 AM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: [FFmpeg-devel] [PATCH v1] avcodec/cbs_vp8: Improve the
> > bitstream position check
> >
> > The VP8 compressed header may not be byte-aligned due to boolean
> > coding. Use bitwise comparison to prevent the potential overread.
> >
> > Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com>
> > ---
> >  libavcodec/cbs_vp8.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c index
> > 065156c248..13acad3724 100644
> > --- a/libavcodec/cbs_vp8.c
> > +++ b/libavcodec/cbs_vp8.c
> > @@ -327,9 +327,10 @@ static int
> > cbs_vp8_read_unit(CodedBitstreamContext
> > *ctx,
> >      if (err < 0)
> >          return err;
> >
> > +    // Position may not be byte-aligned after compressed header; using bits
> > +    // count comparison for accuracy.
> >      pos = get_bits_count(&gbc);
> > -    pos /= 8;
> > -    av_assert0(pos <= unit->data_size);
> > +    av_assert0(pos <= unit->data_size * 8);
> >
> >      frame->data_ref = av_buffer_ref(unit->data_ref);
> >      if (!frame->data_ref)
> 
> Ping reviewers to help to apply.
> 
> The review history can be found here:
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/CH3PR11MB793797554CD
> B411074364733B1742@CH3PR11MB7937.namprd11.prod.outlook.com/

@Ronald (rsbultje@gmail.com), @Andreas (andreas.rheinhardt@outlook.com)

Could you please help to apply these 2 fixes?

[FFmpeg-devel,v1] avcodec/cbs_vp8: Improve the bitstream position check - Patchwork
https://patchwork.ffmpeg.org/project/ffmpeg/patch/DS7PR11MB7949CF2C01F31B4B8597EC61B17A2@DS7PR11MB7949.namprd11.prod.outlook.com/

[FFmpeg-devel,v1] avcodec/cbs_vp8: Use little endian in fixed() - Patchwork
https://patchwork.ffmpeg.org/project/ffmpeg/patch/DS7PR11MB79499AF0B5FB03FBF1876EFCB17A2@DS7PR11MB7949.namprd11.prod.outlook.com/

> 
> > --
> > 2.25.1
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".
Andreas Rheinhardt March 18, 2024, 11:34 a.m. UTC | #3 
Dai, Jianhui J:
> The VP8 compressed header may not be byte-aligned due to boolean
> coding. Use bitwise comparison to prevent the potential overread.
> 
> Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com>
> ---
>  libavcodec/cbs_vp8.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c
> index 065156c248..13acad3724 100644
> --- a/libavcodec/cbs_vp8.c
> +++ b/libavcodec/cbs_vp8.c
> @@ -327,9 +327,10 @@ static int cbs_vp8_read_unit(CodedBitstreamContext *ctx,
>      if (err < 0)
>          return err;
>  
> +    // Position may not be byte-aligned after compressed header; using bits
> +    // count comparison for accuracy.
>      pos = get_bits_count(&gbc);
> -    pos /= 8;
> -    av_assert0(pos <= unit->data_size);
> +    av_assert0(pos <= unit->data_size * 8);

(pos + 7U) / 8 seems better to avoid potential overflow issues
(not an issue atm, but if we ever were to use e.g. 64bit for bitcount of
the GetBit API, then the multiplication on the right could overflow a
32bit size_t).

>  
>      frame->data_ref = av_buffer_ref(unit->data_ref);
>      if (!frame->data_ref)
Dai, Jianhui J March 19, 2024, 6:39 a.m. UTC | #4 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Monday, March 18, 2024 7:35 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v1] avcodec/cbs_vp8: Improve the
> bitstream position check
> 
> Dai, Jianhui J:
> > The VP8 compressed header may not be byte-aligned due to boolean
> > coding. Use bitwise comparison to prevent the potential overread.
> >
> > Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com>
> > ---
> >  libavcodec/cbs_vp8.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c index
> > 065156c248..13acad3724 100644
> > --- a/libavcodec/cbs_vp8.c
> > +++ b/libavcodec/cbs_vp8.c
> > @@ -327,9 +327,10 @@ static int cbs_vp8_read_unit(CodedBitstreamContext
> *ctx,
> >      if (err < 0)
> >          return err;
> >
> > +    // Position may not be byte-aligned after compressed header; using bits
> > +    // count comparison for accuracy.
> >      pos = get_bits_count(&gbc);
> > -    pos /= 8;
> > -    av_assert0(pos <= unit->data_size);
> > +    av_assert0(pos <= unit->data_size * 8);
> 
> (pos + 7U) / 8 seems better to avoid potential overflow issues (not an issue atm,
> but if we ever were to use e.g. 64bit for bitcount of the GetBit API, then the
> multiplication on the right could overflow a 32bit size_t).

Thanks. Fixed it in PATCH v2. Please take a look.

> 
> >
> >      frame->data_ref = av_buffer_ref(unit->data_ref);
> >      if (!frame->data_ref)
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c
index 065156c248..13acad3724 100644
--- a/libavcodec/cbs_vp8.c
+++ b/libavcodec/cbs_vp8.c
@@ -327,9 +327,10 @@  static int cbs_vp8_read_unit(CodedBitstreamContext *ctx,
     if (err < 0)
         return err;
 
+    // Position may not be byte-aligned after compressed header; using bits
+    // count comparison for accuracy.
     pos = get_bits_count(&gbc);
-    pos /= 8;
-    av_assert0(pos <= unit->data_size);
+    av_assert0(pos <= unit->data_size * 8);
 
     frame->data_ref = av_buffer_ref(unit->data_ref);
     if (!frame->data_ref)