2023-05-02 08:37:46

by Gavrilov Ilia

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

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 <[email protected]>
---
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


2023-05-02 11:56:45

by Simon Horman

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

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 <[email protected]>

Reviewed-by: Simon Horman <[email protected]>

2023-05-02 12:03:37

by Simon Horman

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

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 <[email protected]>

Reviewed-by: Simon Horman <[email protected]>

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

2023-05-02 12:29:38

by Horatiu Vultur

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

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 <[email protected]>
> ---
> 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

2023-05-02 13:07:09

by Gavrilov Ilia

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

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 <[email protected]>
Signed-off-by: Ilia.Gavrilov <[email protected]>
---
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

2023-05-02 14:52:50

by Xin Long

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

On Tue, May 2, 2023 at 9:03 AM Gavrilov Ilia <[email protected]> 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 <[email protected]>
> Signed-off-by: Ilia.Gavrilov <[email protected]>
> ---
> 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 <[email protected]>

2023-05-02 15:56:53

by Marcelo Ricardo Leitner

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

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 <[email protected]>
> Signed-off-by: Ilia.Gavrilov <[email protected]>
> ---
> 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

2023-05-02 17:08:56

by Kuniyuki Iwashima

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

From: Gavrilov Ilia <[email protected]>
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 <[email protected]>
> Signed-off-by: Ilia.Gavrilov <[email protected]>
> ---
> 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

2023-05-02 17:57:59

by Marcelo Ricardo Leitner

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

On Tue, May 02, 2023 at 10:05:16AM -0700, Kuniyuki Iwashima wrote:
> From: Gavrilov Ilia <[email protected]>
> 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 <[email protected]>
> > Signed-off-by: Ilia.Gavrilov <[email protected]>
> > ---
> > 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

2023-05-03 09:22:56

by Gavrilov Ilia

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




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


[email protected]
http://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 <[email protected]>
>> 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 <[email protected]>
>>> Signed-off-by: Ilia.Gavrilov <[email protected]>
>>> ---
>>> 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

2023-05-03 10:43:40

by Gavrilov Ilia

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

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 <[email protected]>
---
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

2023-05-03 12:52:00

by Marcelo Ricardo Leitner

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

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

2023-05-03 13:50:42

by Gavrilov Ilia

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

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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Signed-off-by: Ilia.Gavrilov <[email protected]>
---
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

2023-05-03 13:55:08

by Marcelo Ricardo Leitner

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

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 <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>
> Signed-off-by: Ilia.Gavrilov <[email protected]>

Acked-by: Marcelo Ricardo Leitner <[email protected]>

Thx!

2023-05-04 01:51:14

by Jakub Kicinski

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

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