netdev - Re: [PATCH] sctp: fix a potential buffer overflow in sctp_sched_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 May 2023 14:24:59 +0200
From:   Horatiu Vultur <horatiu.vultur@...rochip.com>
To:     Gavrilov Ilia <Ilia.Gavrilov@...otecs.ru>
CC:     Neil Horman <nhorman@...driver.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Xin Long <lucien.xin@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>
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 <Ilia.Gavrilov@...otecs.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