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