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.



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ