netdev - Re: [PATCH net-next] tcp: fix incorrect parameter validation in the do_tcp

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] [day] [month] [year] [list]

Message-ID: <5c65a5be-ba95-4420-b755-aabc5ae7559b@infotecs.ru>
Date: Thu, 7 Mar 2024 14:17:32 +0000
From: Gavrilov Ilia <Ilia.Gavrilov@...otecs.ru>
To: Simon Horman <horms@...nel.org>
CC: Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	"David S. Miller" <davem@...emloft.net>, David Ahern <dsahern@...nel.org>,
	Jakub Kicinski <kuba@...nel.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 net-next] tcp: fix incorrect parameter validation in the
 do_tcp_getsockopt() function




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


Ilia.Gavrilov@...otecs.ru
www.infotecs.ru


On 3/7/24 11:40, Simon Horman wrote:
> On Wed, Mar 06, 2024 at 11:54:40AM +0000, Gavrilov Ilia wrote:
>> On 3/6/24 14:36, Paolo Abeni wrote:
>>> The above is incorrect, as the 'len' variable is a signed integer
>>
>> I mean, if 'len' is negative then after this expression
>> len = min_t(unsigned int, len, sizeof(int));
>> the 'len' variable will be equal to sizeof(int) == 4
>> and the statement
>> if (len < 0) return -EINVAL;
>> might be unreachable during program execution.
>
> Hi Gavrilov and Paolo,
>
> I could be missing something obvious but it seems to me that this is correct.
> Although perhaps we could try rewording the patch description to
> make things a bit clearer. Here is my attempt at that:
>
>     The 'len' variable can't be negative when assigned the result of
>     'min_t' because all 'min_t' parameters are cast to unsigned int,
>     and then the minimum one is chosen.
>
>     To fix the logic, check 'len' as read from 'optlen',
>     where the types of relevant variables are (signed) int.
>
> FWIIW, I see four similar patches on netdev this morning.
> It does seem to me that they are all valid fixes.
> But if they need to be reposted, or there are more coming,
> then I think it would be useful to bundle them up,
> say into batches of 10, and send as patch-sets.
>
> This may help with fragmentation of review of what seems
> to be the same change in multiple places.
>
>

Hi Simon, thank you for your answer.

I'll reword the patch description and repost a series of patches in V2.
I also found a couple of places with the same problem.