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
| ||
|
Message-ID: <95bc9b12-9e1e-5054-c2df-ad9201d94ed5@infotecs.ru> Date: Wed, 3 May 2023 09:08:17 +0000 From: Gavrilov Ilia <Ilia.Gavrilov@...otecs.ru> To: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, Kuniyuki Iwashima <kuniyu@...zon.com> CC: "davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>, "lucien.xin@...il.com" <lucien.xin@...il.com>, "lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "nhorman@...driver.com" <nhorman@...driver.com>, "pabeni@...hat.com" <pabeni@...hat.com>, "simon.horman@...igine.com" <simon.horman@...igine.com> 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 Ilia.Gavrilov@...otecs.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: >> From: Gavrilov Ilia <Ilia.Gavrilov@...otecs.ru> >> 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@...igine.com> >>> Signed-off-by: Ilia.Gavrilov <Ilia.Gavrilov@...otecs.ru> >>> --- >>> 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
Powered by blists - more mailing lists