[FFmpeg-devel] [PATCH] opus_silk: fix out of array read in silk_lsf2lpc

[Michael Niedermayer]

On Mon, Dec 14, 2015 at 08:43:38PM +0100, Andreas Cadhalpun wrote:
> On 14.12.2015 02:38, Michael Niedermayer wrote:
> > On Sun, Dec 13, 2015 at 10:51:31PM +0100, Andreas Cadhalpun wrote:
> >> nlsf can be negative, but a negative index for silk_cosine doesn't work.
> >> ---
> >>  libavcodec/opus_silk.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/opus_silk.c b/libavcodec/opus_silk.c
> >> index 841d1ed..3ac83b8 100644
> >> --- a/libavcodec/opus_silk.c
> >> +++ b/libavcodec/opus_silk.c
> >> @@ -941,7 +941,7 @@ static void silk_lsf2lpc(const int16_t nlsf[16], float lpcf[16], int order)
> >>  
> >>      /* convert the LSFs to LSPs, i.e. 2*cos(LSF) */
> >>      for (k = 0; k < order; k++) {
> >> -        int index = nlsf[k] >> 8;
> >> +        int index = FFABS(nlsf[k]) >> 8;
> >>          int offset = nlsf[k] & 255;
> > 
> > this looks a bit strange
> > 
> > if nlsf[] is allowed to be negative then i would have expected that
> > both nlsf[] would use the absolute value or if its nt allowed to be
> > negative then why is it ?
> 
> Actually the specification says it shouldn't be negative.
> It's negative due to an overflow, when an intermediary uint16_t value
> larger than INT16_MAX is assigned to nlsf.
> 
> > does the spec explain what should be done in this case ?
> 
> Not directly, but it implies that the overflow shouldn't happen [1]:
>    Then, for each
>    value of k from 0 to d_LPC-1, NLSF_Q15[k] is set to
> 
>              max(NLSF_Q15[k], NLSF_Q15[k-1] + NDeltaMin_Q15[k])
> 
>    Next, for each value of k from d_LPC-1 down to 0, NLSF_Q15[k] is set
>    to
> 
>             min(NLSF_Q15[k], NLSF_Q15[k+1] - NDeltaMin_Q15[k+1])
> 
> > if not, its authors may want to amend it to clarify if this is
> > invalid or undefined or something specific should be done
> 
> I think the specification is fine, just not our code.
> Attached is a patch fixing this.
> 
> Best regards,
> Andreas
> 
> 1: https://tools.ietf.org/html/rfc6716

>  opus_silk.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> cc0c02e14c1bda0ab35813c8d4629e742af7d23f  0001-opus_silk-fix-int16_t-overflow-in-silk_stabilize_lsf.patch
> From 958789a66e6f55e05ab3d8e945b8ff899680c073 Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> Date: Mon, 14 Dec 2015 20:31:41 +0100
> Subject: [PATCH] opus_silk: fix int16_t overflow in silk_stabilize_lsf
> 
> nlsf[i - 1] + min_delta[i] can be larger than INT16_MAX, causing nlsf to
> be set to a negative value. However, it is not supposed to be negative
> and if it is, it causes an out of bounds read in silk_lsf2lpc.
> 
> Since min_delta is unsigned, the overflow only happens when the result
> of the addition is assigned to nlsf, so that the FFMIN solves the
> problem.
> 
> Even though the specification implies that the value of nlfs can be
> larger than INT16_MAX at this intermediary point, it is reduced to the
> int16_t range in the next loop, the result of which doesn't change if
> the too large intermediary values are replaced by INT16_MAX.
> 
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> ---
>  libavcodec/opus_silk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

should be ok

thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151214/6ae7a5ce/attachment.sig>