[PATCH] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[PATCH] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[Gavrilov Ilia]

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

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

Re: [PATCH] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[Simon Horman]

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>

Re: [PATCH] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[Simon Horman]

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
> 

Re: [PATCH] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[Horatiu Vultur]

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

-- 
/Horatiu

[PATCH net v2] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[Gavrilov Ilia]

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")
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru>
---
V2:
 - Change the order of local variables 
 - Specify the target tree in the subject
 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..4d076a9b8592 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 *old = asoc->outqueue.sched;
 	struct sctp_datamsg *msg = NULL;
+	struct sctp_sched_ops *n;
 	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

Re: [PATCH net v2] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[Xin Long]

On Tue, May 2, 2023 at 9:03 AM Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru> 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")
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru>
> ---
> V2:
>  - Change the order of local variables
>  - Specify the target tree in the subject
>  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..4d076a9b8592 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 *old = asoc->outqueue.sched;
>         struct sctp_datamsg *msg = NULL;
> +       struct sctp_sched_ops *n;
>         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

Reviewed-by: Xin Long <lucien.xin@gmail.com>

Re: [PATCH net v2] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[Marcelo Ricardo Leitner]

On Tue, May 02, 2023 at 01:03:24PM +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.

Buffer overflow? Or you mean, read beyond the buffer and possibly a
bad pointer dereference? Because the buffer itself is not written to.

> 
> 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")
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru>
> ---
> V2:
>  - Change the order of local variables 
>  - Specify the target tree in the subject
>  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..4d076a9b8592 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 *old = asoc->outqueue.sched;
>  	struct sctp_datamsg *msg = NULL;
> +	struct sctp_sched_ops *n;
>  	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

Re: [PATCH net v2] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[Kuniyuki Iwashima]

From:   Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru>
Date:   Tue, 2 May 2023 13:03:24 +0000
> The 'sched' index value must be checked before accessing an element
> of the 'sctp_sched_ops' array. Otherwise, it can lead to buffer overflow.

OOB access ?
But it's not true because it does not happen in the first place.

> 
> 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")
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru>
> ---
> V2:
>  - Change the order of local variables 
>  - Specify the target tree in the subject
>  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..4d076a9b8592 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 *old = asoc->outqueue.sched;
>  	struct sctp_datamsg *msg = NULL;
> +	struct sctp_sched_ops *n;
>  	struct sctp_chunk *ch;
>  	int i, ret = 0;
>  
> -	if (old == n)
> -		return ret;
> -
>  	if (sched > SCTP_SS_MAX)
>  		return -EINVAL;

I'd just remove this check instead because the same test is done
in the caller side, sctp_setsockopt_scheduler(), and this errno
is never returned.

This unnecessary test confuses a reader like sched could be over
SCTP_SS_MAX here.

Since the OOB access does not happen, I think this patch should
go to net-next without the Fixes tag after the merge window.

Thanks,
Kuniyuki


>  
> +	n = sctp_sched_ops[sched];
> +	if (old == n)
> +		return ret;
> +
>  	if (old)
>  		sctp_sched_free_sched(&asoc->stream);
>  
> -- 
> 2.30.2

Re: [PATCH net v2] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[Marcelo Ricardo Leitner]

On Tue, May 02, 2023 at 10:05:16AM -0700, Kuniyuki Iwashima wrote:
> From:   Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru>
> Date:   Tue, 2 May 2023 13:03:24 +0000
> > The 'sched' index value must be checked before accessing an element
> > of the 'sctp_sched_ops' array. Otherwise, it can lead to buffer overflow.
> 
> OOB access ?

My thought as well.

> But it's not true because it does not happen in the first place.
> 
> > 
> > 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")
> > Reviewed-by: Simon Horman <simon.horman@corigine.com>
> > Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru>
> > ---
> > V2:
> >  - Change the order of local variables 
> >  - Specify the target tree in the subject
> >  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..4d076a9b8592 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 *old = asoc->outqueue.sched;
> >  	struct sctp_datamsg *msg = NULL;
> > +	struct sctp_sched_ops *n;
> >  	struct sctp_chunk *ch;
> >  	int i, ret = 0;
> >  
> > -	if (old == n)
> > -		return ret;
> > -
> >  	if (sched > SCTP_SS_MAX)
> >  		return -EINVAL;
> 
> I'd just remove this check instead because the same test is done
> in the caller side, sctp_setsockopt_scheduler(), and this errno
> is never returned.
> 
> This unnecessary test confuses a reader like sched could be over
> SCTP_SS_MAX here.

It's actualy better to keep the test here and remove it from the
callers: they don't need to know the specifics, and further new calls
will be protected already.

> 
> Since the OOB access does not happen, I think this patch should
> go to net-next without the Fixes tag after the merge window.

Yup.

> 
> Thanks,
> Kuniyuki
> 
> 
> >  
> > +	n = sctp_sched_ops[sched];
> > +	if (old == n)
> > +		return ret;
> > +
> >  	if (old)
> >  		sctp_sched_free_sched(&asoc->stream);
> >  
> > -- 
> > 2.30.2

Re: [PATCH net v2] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[Gavrilov Ilia]

С уважением,
Илья Гаврилов
Ведущий программист
Отдел разработки
АО "ИнфоТеКС" в г. Санкт-Петербург
127287, г. Москва, Старый Петровско-Разумовский проезд, дом 1/23, стр. 1
T: +7 495 737-61-92 ( доб. 4921)
Ф: +7 495 737-72-78


Ilia.Gavrilov@infotecs.ru
www.infotecs.ru


On 5/2/23 20:49, Marcelo Ricardo Leitner wrote:
> On Tue, May 02, 2023 at 10:05:16AM -0700, Kuniyuki Iwashima wrote:
>> From:   Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru>
>> Date:   Tue, 2 May 2023 13:03:24 +0000
>>> The 'sched' index value must be checked before accessing an element
>>> of the 'sctp_sched_ops' array. Otherwise, it can lead to buffer overflow.
>>
>> OOB access ?
>
> My thought as well.
>

I'm sorry. Yes, I meant out-of-bounds access.

>> But it's not true because it does not happen in the first place.
>>
>>>
>>> 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")
>>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>>> Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru>
>>> ---
>>> V2:
>>>   - Change the order of local variables
>>>   - Specify the target tree in the subject
>>>   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..4d076a9b8592 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 *old = asoc->outqueue.sched;
>>>   struct sctp_datamsg *msg = NULL;
>>> +struct sctp_sched_ops *n;
>>>   struct sctp_chunk *ch;
>>>   int i, ret = 0;
>>>
>>> -if (old == n)
>>> -return ret;
>>> -
>>>   if (sched > SCTP_SS_MAX)
>>>   return -EINVAL;
>>
>> I'd just remove this check instead because the same test is done
>> in the caller side, sctp_setsockopt_scheduler(), and this errno
>> is never returned.
>>
>> This unnecessary test confuses a reader like sched could be over
>> SCTP_SS_MAX here.
>
> It's actualy better to keep the test here and remove it from the
> callers: they don't need to know the specifics, and further new calls
> will be protected already.
>

I agree that the check should be removed, but I think it's better to
keep the test on the calling side, because params->assoc_value is set as
the default "stream schedule" for the socket and it needs to be checked too.

static int sctp_setsockopt_scheduler(..., struct sctp_assoc_value
*params, ...)
{
...
   if (params->assoc_id == SCTP_FUTURE_ASSOC ||
       params->assoc_id == SCTP_ALL_ASSOC)
       sp->default_ss = params->assoc_value;
...
}

>>
>> Since the OOB access does not happen, I think this patch should
>> go to net-next without the Fixes tag after the merge window.
>
> Yup.
>
>>
>> Thanks,
>> Kuniyuki
>>
>>
>>>
>>> +n = sctp_sched_ops[sched];
>>> +if (old == n)
>>> +return ret;
>>> +
>>>   if (old)
>>>   sctp_sched_free_sched(&asoc->stream);
>>>
>>> --
>>> 2.30.2

[PATCH net v3] sctp: remove unncessary check in sctp_sched_set_sched()

[Gavrilov Ilia]

The value of the 'sched' parameter of the function 'sctp_sched_set_sched'
is checked on the calling side, so the internal check can be removed

Found by InfoTeCS on behalf of Linux Verification Center
(linuxtesting.org) with SVACE.

Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru>
---
V2:
 - Change the order of local variables 
 - Specify the target tree in the subject
V3:
 - Change description
 - Remove 'fixes'
 net/sctp/stream_sched.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/sctp/stream_sched.c b/net/sctp/stream_sched.c
index 330067002deb..52d308bdb469 100644
--- a/net/sctp/stream_sched.c
+++ b/net/sctp/stream_sched.c
@@ -155,9 +155,6 @@ int sctp_sched_set_sched(struct sctp_association *asoc,
 	if (old == n)
 		return ret;
 
-	if (sched > SCTP_SS_MAX)
-		return -EINVAL;
-
 	if (old)
 		sctp_sched_free_sched(&asoc->stream);
 
-- 
2.30.2

Re: [PATCH net v2] sctp: fix a potential buffer overflow in sctp_sched_set_sched()

[Marcelo Ricardo Leitner]

On Wed, May 03, 2023 at 09:08:17AM +0000, Gavrilov Ilia wrote:
> >> This unnecessary test confuses a reader like sched could be over
> >> SCTP_SS_MAX here.
> >
> > It's actualy better to keep the test here and remove it from the
> > callers: they don't need to know the specifics, and further new calls
> > will be protected already.
> >
> 
> I agree that the check should be removed, but I think it's better to
> keep the test on the calling side, because params->assoc_value is set as
> the default "stream schedule" for the socket and it needs to be checked too.
> 
> static int sctp_setsockopt_scheduler(..., struct sctp_assoc_value
> *params, ...)
> {
> ...
>    if (params->assoc_id == SCTP_FUTURE_ASSOC ||
>        params->assoc_id == SCTP_ALL_ASSOC)
>        sp->default_ss = params->assoc_value;
> ...
> }

Good point. But then, don't remove the check. Instead, just fix that
ordering and make it less confusing.  Otherwise you will be really
making it prone to the OOB if a new call gets added that doesn't
validate the parameter.

Thanks,
Marcelo

[PATCH net v4] sctp: fix a potential OOB access in sctp_sched_set_sched()

[Gavrilov Ilia]

The 'sched' index value must be checked before accessing an element
of the 'sctp_sched_ops' array. Otherwise, it can lead to OOB access.

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.

Reviewed-by: Xin Long <lucien.xin@gmail.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru>
---
V2:
 - Change the order of local variables 
 - Specify the target tree in the subject
V3:
 - Change description
 - Remove 'fixes'
V4:
 - revert to V2
 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..4d076a9b8592 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 *old = asoc->outqueue.sched;
 	struct sctp_datamsg *msg = NULL;
+	struct sctp_sched_ops *n;
 	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

Re: [PATCH net v4] sctp: fix a potential OOB access in sctp_sched_set_sched()

[Marcelo Ricardo Leitner]

On Wed, May 03, 2023 at 01:37:59PM +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 OOB access.
> 
> 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.
> 
> Reviewed-by: Xin Long <lucien.xin@gmail.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@infotecs.ru>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Thx!

Re: [PATCH net v4] sctp: fix a potential OOB access in sctp_sched_set_sched()

[Jakub Kicinski]

On Wed, 3 May 2023 13:37:59 +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 OOB access.
> 
> Note that it's harmless since the 'sched' parameter is checked before
> calling 'sctp_sched_set_sched'.

Not a fix, so it needs to wait for net-next to open, see below.
When you repost please do so separately, not in the existing thread.

## Form letter - net-next-closed

The merge window for v6.3 has begun and therefore net-next is closed
for new drivers, features, code refactoring and optimizations.
We are currently accepting bug fixes only.

Please repost when net-next reopens after May 8th.

RFC patches sent for review only are obviously welcome at any time.

See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
-- 
pw-bot: defer