С уважением,
Илья Гаврилов
Ведущий программист
Отдел разработки
АО "ИнфоТеКС" в г. Санкт-Петербург
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:

quoted

From:   Gavrilov Ilia <redacted>
Date:   Tue, 2 May 2023 13:03:24 +0000

quoted

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.

quoted

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

quoted

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 <redacted>
Signed-off-by: Ilia.Gavrilov <redacted>
---
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;
...
}

quoted

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.

quoted

Thanks,
Kuniyuki

quoted

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

--
2.30.2