Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 2 May 2023 14:24:59 +0200
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru>
CC:     Neil Horman <nhorman@tuxdriver.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
        Xin Long <lucien.xin@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "linux-sctp@vger.kernel.org" <linux-sctp@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "lvc-project@linuxtesting.org" <lvc-project@linuxtesting.org>
Subject: Re: [PATCH] sctp: fix a potential buffer overflow in
 sctp_sched_set_sched()
Message-ID: <20230502122459.inxuqa5rt3iluec4@soft-dev3-1>
References: <20230502082622.2392659-1-Ilia.Gavrilov@infotecs.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <20230502082622.2392659-1-Ilia.Gavrilov@infotecs.ru>
Precedence: bulk

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