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

Message-ID: <20240307084014.GH281974@kernel.org>
Date: Thu, 7 Mar 2024 08:40:14 +0000
From: Simon Horman <horms@...nel.org>
To: Gavrilov Ilia <Ilia.Gavrilov@...otecs.ru>
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

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.