Received-SPF: pass (google.com: domain of linux-kernel+bounces-95154-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 7 Mar 2024 08:40:14 +0000
From: Simon Horman <horms@kernel.org>
To: Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru>
Cc: Paolo Abeni <pabeni@redhat.com>, Eric Dumazet <edumazet@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	David Ahern <dsahern@kernel.org>, Jakub Kicinski <kuba@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"lvc-project@linuxtesting.org" <lvc-project@linuxtesting.org>
Subject: Re: [PATCH net-next] tcp: fix incorrect parameter validation in the
 do_tcp_getsockopt() function
Message-ID: <20240307084014.GH281974@kernel.org>
References: <20240306095430.1782163-1-Ilia.Gavrilov@infotecs.ru>
 <095ce1d0f2cd6771b30ab1d73ee6aa8e8460c7c8.camel@redhat.com>
 <e8b2287f-bf25-4a95-aef2-58067c893b4f@infotecs.ru>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e8b2287f-bf25-4a95-aef2-58067c893b4f@infotecs.ru>

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.