| Message ID | 20230502082622.2392659-1-Ilia.Gavrilov@infotecs.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sctp: fix a potential buffer overflow in sctp_sched_set_sched() | expand |
On Tue, May 02, 2023 at 08:26:30AM +0000, Gavrilov Ilia wrote: > The 'sched' index value must be checked before accessing an element > of the 'sctp_sched_ops' array. Otherwise, it can lead to buffer overflow. > > Note that it's harmless since the 'sched' parameter is checked before > calling 'sctp_sched_set_sched'. > > Found by InfoTeCS on behalf of Linux Verification Center > (linuxtesting.org) with SVACE. > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations") > Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On Tue, May 02, 2023 at 08:26:30AM +0000, Gavrilov Ilia wrote: > The 'sched' index value must be checked before accessing an element > of the 'sctp_sched_ops' array. Otherwise, it can lead to buffer overflow. > > Note that it's harmless since the 'sched' parameter is checked before > calling 'sctp_sched_set_sched'. > > Found by InfoTeCS on behalf of Linux Verification Center > (linuxtesting.org) with SVACE. > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations") > Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru> Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > net/sctp/stream_sched.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/net/sctp/stream_sched.c b/net/sctp/stream_sched.c > index 330067002deb..a339917d7197 100644 > --- a/net/sctp/stream_sched.c > +++ b/net/sctp/stream_sched.c > @@ -146,18 +146,19 @@ static void sctp_sched_free_sched(struct sctp_stream *stream) > int sctp_sched_set_sched(struct sctp_association *asoc, > enum sctp_sched_type sched) > { > - struct sctp_sched_ops *n = sctp_sched_ops[sched]; > + struct sctp_sched_ops *n; nit: reverse xmas tree - longest line to shortest - for local variable declarations in networking code. > struct sctp_sched_ops *old = asoc->outqueue.sched; > struct sctp_datamsg *msg = NULL; > struct sctp_chunk *ch; > int i, ret = 0; > > - if (old == n) > - return ret; > - > if (sched > SCTP_SS_MAX) > return -EINVAL; > > + n = sctp_sched_ops[sched]; > + if (old == n) > + return ret; > + > if (old) > sctp_sched_free_sched(&asoc->stream); > > -- > 2.30.2 >
The 05/02/2023 08:26, Gavrilov Ilia wrote: Hi, > > The 'sched' index value must be checked before accessing an element > of the 'sctp_sched_ops' array. Otherwise, it can lead to buffer overflow. > > Note that it's harmless since the 'sched' parameter is checked before > calling 'sctp_sched_set_sched'. If the 'sched' parameter is already checked, is it not better to remove the check from this function? > > Found by InfoTeCS on behalf of Linux Verification Center > (linuxtesting.org) with SVACE. > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations") I am not sure how much this is net material because as you said, this issue can't happen. But don't forget to specify the target tree in the subject. You can do that when creating the patch using: git format-patch ... --subject-prefix "PATCH net" > Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru> > --- > net/sctp/stream_sched.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/net/sctp/stream_sched.c b/net/sctp/stream_sched.c > index 330067002deb..a339917d7197 100644 > --- a/net/sctp/stream_sched.c > +++ b/net/sctp/stream_sched.c > @@ -146,18 +146,19 @@ static void sctp_sched_free_sched(struct sctp_stream *stream) > int sctp_sched_set_sched(struct sctp_association *asoc, > enum sctp_sched_type sched) > { > - struct sctp_sched_ops *n = sctp_sched_ops[sched]; > + struct sctp_sched_ops *n; > struct sctp_sched_ops *old = asoc->outqueue.sched; > struct sctp_datamsg *msg = NULL; > struct sctp_chunk *ch; > int i, ret = 0; > > - if (old == n) > - return ret; > - > if (sched > SCTP_SS_MAX) > return -EINVAL; > > + n = sctp_sched_ops[sched]; > + if (old == n) > + return ret; > + > if (old) > sctp_sched_free_sched(&asoc->stream); > > -- > 2.30.2
diff --git a/net/sctp/stream_sched.c b/net/sctp/stream_sched.c index 330067002deb..a339917d7197 100644 --- a/net/sctp/stream_sched.c +++ b/net/sctp/stream_sched.c @@ -146,18 +146,19 @@ static void sctp_sched_free_sched(struct sctp_stream *stream) int sctp_sched_set_sched(struct sctp_association *asoc, enum sctp_sched_type sched) { - struct sctp_sched_ops *n = sctp_sched_ops[sched]; + struct sctp_sched_ops *n; struct sctp_sched_ops *old = asoc->outqueue.sched; struct sctp_datamsg *msg = NULL; struct sctp_chunk *ch; int i, ret = 0; - if (old == n) - return ret; - if (sched > SCTP_SS_MAX) return -EINVAL; + n = sctp_sched_ops[sched]; + if (old == n) + return ret; + if (old) sctp_sched_free_sched(&asoc->stream);
The 'sched' index value must be checked before accessing an element of the 'sctp_sched_ops' array. Otherwise, it can lead to buffer overflow. Note that it's harmless since the 'sched' parameter is checked before calling 'sctp_sched_set_sched'. Found by InfoTeCS on behalf of Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations") Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru> --- net/sctp/stream_sched.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)