Date:   Sun, 19 Feb 2023 15:14:19 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Maxim Korotkov <korotkov.maxim.s@gmail.com>
Cc:     Michael Chan <michael.chan@broadcom.com>,
        Pavan Chebbi <pavan.chebbi@broadcom.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, lvc-project@linuxtesting.org
Subject: Re: [PATCH v2] bnxt: avoid overflow in bnxt_get_nvram_directory()
Message-ID: <Y/Iuu9SiAxh7qhJM@corigine.com>
References: <20230219084656.17926-1-korotkov.maxim.s@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230219084656.17926-1-korotkov.maxim.s@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?oI72xbgGgXFxW3Fey7Wmz3eFtfsq/45Rg0BwbVHteMObgZO2sx9cjtiSvgYZ?=
 =?us-ascii?Q?XXTg0j4LgX7KURxARZOV2xACfwuDbEul6cA835MvpjZ4LrfWB70Cw+C1gKSP?=
 =?us-ascii?Q?NcIvmt47wwp6t9Grngejokvl0oyQvJfT3ZrxxhhJffUQ+h+DrvinWoMQVgTl?=
 =?us-ascii?Q?e/teJ/C9RYKBnm9SfJsF3P68domuwX8tzGEvQLJHRNBniZ4nStV0XFuJOOT3?=
 =?us-ascii?Q?e5K2vjxCEJF6LQXkbLs8yFXxIuZ1qUVOIdNKwrT2PvOLC6x8aj5daHuAvfob?=
 =?us-ascii?Q?tt93uM5yJHISNNyAVEUXn/cUdeVk04SQaU9mITYq4XR4S912j+DvBbkAD7md?=
 =?us-ascii?Q?+ueWzD8ny/bXb+ZLmN8QLqDVlnmacnE87Do6HNM+YGFz/NMLipuc4MBfxLk4?=
 =?us-ascii?Q?wj+BTjkCD8Va2IXzKj1J9NhF9FBFv3/w6+vUYmu1vht3zw2HhaIMIL0QeYdq?=
 =?us-ascii?Q?hzTS6eqJEyrajtHSXGSpsO+o3MQXfzp/1lRfASrB/xVzSi7O6RV9AjCkOIKZ?=
 =?us-ascii?Q?WLoxs87hW6BWomIbL6xEkTAIQqv8OlBYX4DpB67CK55MqWmeuz4CQ5tfgSGT?=
 =?us-ascii?Q?AmNXgXH771cYHHZzE6H4a2lE37T8IX806U3BIsus2I88ute9WCbBxkvTbw9y?=
 =?us-ascii?Q?Crxmrvymf5H9NjOs58N4oQ4w2kbOQKFNY7cNTopT3DhOcvO7QVrEHyCszBKq?=
 =?us-ascii?Q?rjVuQJ/Zj6zNysixkpebCO7I1gmHqouYLl9hbo6hO8glXRY73dVFpbG13nuH?=
 =?us-ascii?Q?8WXLlZohyoUbp8NkAomGP8O7M19LUWXpUxSfCWzWz05k3CzCItIR4tVbl9W8?=
 =?us-ascii?Q?2GmATnjE9fNPQzTaDQ9nRRdmETUBEJHP84zFlcQQ9kXapf7A5MscAQme/ZNT?=
 =?us-ascii?Q?CvWIIhjOD6BnTLN9Q0NdGa/F1nEA4CjhFbelrGNzBD8uUxlDOUWoC2klQtz7?=
 =?us-ascii?Q?fSPm9Da7cGsZPKBiPlR3bBRLgz22RGQvKWeuBSmPR56XA1ycjORacoVyocb7?=
 =?us-ascii?Q?ND9U9PL85qL1bMTYPjSc0bzcFXxZhi1cQ0Li+wBOM5wbtaeA1OL/X2NzELsr?=
 =?us-ascii?Q?72/NJ85+pUr/8fLYHgk0Y5DzUeapduAC0Hu7W8cTxB3KpsSixOCAx2xOmhi+?=
 =?us-ascii?Q?SRoKwLHnixYFj+VBX9xlFINrLf/Fl9eVUIZxcuy3BfMp9ttby8SzPQbP8+kt?=
 =?us-ascii?Q?Cgf1q8v6GfZ7iQBFVSh20lRcPHVPVIR+CANmCOMkE/ik9PUpj+UlTQ2j/T/p?=
 =?us-ascii?Q?QCViFWDsjoHqeKVQQmszHvqtV4hnhtW2YySQDiC20cZ/Rh6fu+5Y/DVAlgoM?=
 =?us-ascii?Q?1Uoflc+uEpoddRVbDZTFoWH+QUVgj7RiL0j106FK8Nx2mlJFpf8jOyCnRdqI?=
 =?us-ascii?Q?IVB5YQ/Rvd7UH+6A07r2QIxdoTLNJcsJa5a46McZznEa0qGEJO/zoPPhjQAt?=
 =?us-ascii?Q?4wX/dlpazHgHgfkeNqwcmaNB7Qx/cNDV0Q60dumyhKPXnxRk8pFee57dxodK?=
 =?us-ascii?Q?hkNUqG4Vei6hrXktblgT/56XbJxVlDvfcxtv5Qv2QTValKBfT0gkLDz0J/sT?=
 =?us-ascii?Q?CIo+DINdWopUMElDws+MUBEZL2mxfWDSvbvaRo6kYJffuHhWKYONAG+IvH9I?=
 =?us-ascii?Q?qr8Ra2H2ARy8+tGQoTo3o11dHyrkQFuVjTQc/jGfE4nOQzNQ6SyWbDASIHIZ?=
 =?us-ascii?Q?RMoWyQ=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b18f73b3-d001-49de-830d-08db12839b51
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Feb 2023 14:14:26.1382
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 79zNgAB+eFq79LSe5bAyer+1nKpx6eKKL8mkPf2btA63/xQRcsvmmBcfsxBnEYeGK1QLQYs4NTwWG44x3FbbM0O6lpmGBKpI1df+UzGUl9U=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR13MB6152
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Feb 19, 2023 at 11:46:56AM +0300, Maxim Korotkov wrote:
> The value of an arithmetic expression is subject
> of possible overflow due to a failure to cast operands to a larger data
> type before performing arithmetic. Used macro for multiplication instead
> operator for avoiding overflow.
> 
> Found by Security Code and Linux Verification
> Center (linuxtesting.org) with SVACE.
> 
> Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
> Signed-off-by: Maxim Korotkov <korotkov.maxim.s@gmail.com>
> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>

I agree that it is correct to use mul_u32_u32() for multiplication
of two u32 entities where the result is 64bit, avoiding overflow.

And I agree that the fixes tag indicates the commit where the code
in question was introduced.

However, it is not clear to me if this is a theoretical bug
or one that can manifest in practice - I think it implies that
buflen really can be > 4Gbytes.

And thus it is not clear to me if this patch should be for 'net' or
'net-next'.

> ---
> changelog:
> - added "fixes" tag.
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index ec573127b707..696f32dfe41f 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -2862,7 +2862,7 @@ static int bnxt_get_nvram_directory(struct net_device *dev, u32 len, u8 *data)
>  	if (rc)
>  		return rc;
>  
> -	buflen = dir_entries * entry_length;
> +	buflen = mul_u32_u32(dir_entries, entry_length);
>  	buf = hwrm_req_dma_slice(bp, req, buflen, &dma_handle);
>  	if (!buf) {
>  		hwrm_req_drop(bp, req);
> -- 
> 2.37.2
>