From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 96AEC657D5; Thu, 7 Mar 2024 08:40:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709800819; cv=none; b=aECOxRyzuOiwDiEu9lqoKqeFBv9BYcKJfEqds0n+DjTACvNM5riezYc6l/PjiWWz5Fw9Rk2p56vGvVDvTS17WGxHrrxRCUSwjmcf6KRDJWK0Hv2ugK6F9ogbUgo34wYyn4CagakN5vctYMOyU2bhcjLZmNwzqE/7bfIxrtcEp1I= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709800819; c=relaxed/simple; bh=ruvxBhPt5E6gAvEp7k33Gsr1p4H69XU+cVmoNjLZvFQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=E76XCt7zWWfD9rtxuBbX1C0I42I2xXSZ+lWuKPn+KA1WViuoh2IfxxSiB3/8yprnIi8dUrG73Yeb905+sKJntH5XOQAVbzdwOzHbFhKC7KwjGOtsnM4CnjqKUteBTJt0YHOkztp4mx3Qea6ijWOTUHjQkk2kEJiccIFylI0mrhw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=R6Qb+rAd; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="R6Qb+rAd" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 198B4C433C7; Thu, 7 Mar 2024 08:40:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1709800819; bh=ruvxBhPt5E6gAvEp7k33Gsr1p4H69XU+cVmoNjLZvFQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=R6Qb+rAd1Z5sjC3wf/T1VUClaXzh6muwyahcdADPCchk1F3O3lbsACdncdq3e2kG8 rPtLcznbFdhHuLYEROehisYGeIIZaA4rqilOagZyq6u9ajqB4WWFIdps6MShW4D7Iy GFWkI2uVUSOIyIWwgnz/FKQHsL7HH2YOvM5+hHxpydmGQ0TvPtbFjg6kU7q3Qg1sqQ wxEyJzQiIJLFWl5CNw/x1WJyBNpVMz9JIcwpsVwtmbPzl+cnJmk17J17SjBi3Dzs6b CI25b0HJm1FIGPbB4VDSf38kbiD8geOmWy+UbF8XBlI6kQw/KL34xnNiyYFxVt/ud2 ubxszuRIONuKw== Date: Thu, 7 Mar 2024 08:40:14 +0000 From: Simon Horman To: Gavrilov Ilia Cc: Paolo Abeni , Eric Dumazet , "David S. Miller" , David Ahern , Jakub Kicinski , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.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> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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.