diff mbox series

sctp: fix a potential buffer overflow in sctp_sched_set_sched()

Message ID 20230502082622.2392659-1-Ilia.Gavrilov@infotecs.ru
State New
Headers
Series sctp: fix a potential buffer overflow in sctp_sched_set_sched() |

Commit Message

Gavrilov Ilia May 2, 2023, 8:26 a.m. UTC 
  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(-)

Comments

Simon Horman May 2, 2023, 11:48 a.m. UTC | #1 
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>
Simon Horman May 2, 2023, 11:56 a.m. UTC | #2 
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
>
Horatiu Vultur May 2, 2023, 12:24 p.m. UTC | #3 
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 mbox series

Patch

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);