Message-ID: <ee0aa756-4a9c-1d7a-4179-78024e41d37e@intel.com>
Date:   Tue, 7 Mar 2023 18:47:14 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH] xsk: Add missing overflow check in xdp_umem_reg
Content-Language: en-US
To:     Kal Conley <kal.conley@dectris.com>
CC:     =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
        Magnus Karlsson <magnus.karlsson@intel.com>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Jonathan Lemon <jonathan.lemon@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "Alexei Starovoitov" <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        John Fastabend <john.fastabend@gmail.com>,
        <netdev@vger.kernel.org>, <bpf@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20230307172306.786657-1-kal.conley@dectris.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230307172306.786657-1-kal.conley@dectris.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NkV3amNnVHFPSit1eWNVLzd0NTZpT3Y4algrZE9GekF3OWd5N1lZVnV5QjFi?=
 =?utf-8?B?UCtSdkdKd0o4V2xyUW8wK2FnVWpuaUtqdzJIOUM3YkMzYkt0MTM1T3ptNVE2?=
 =?utf-8?B?YWtzbnhJTnRsNkk3SW1LK0o2MnRGczdhTHlqN2dtQklGRUhqc3cvVFVFOEJC?=
 =?utf-8?B?Rm1yc1VmUmhXWmhkZ0dweldMTzZ4K0cyTkZkdEtZSmZ6N0NyKzNZc2l1SXdQ?=
 =?utf-8?B?RENjaTZCSktSbVg1MTlZM3JLTElLM3BZVTNIRlFySEhRdXNZRWUxVE9GR2NE?=
 =?utf-8?B?bzNYQXFYOVdLckVIMzBpZHVVb3dSY2tBcS81dWEwNk5URnN4SUI3Y3NmOXY1?=
 =?utf-8?B?UDN5a2JBemE1UW5IS25BM3pPUnVSdEtPTGdXQ3R3SEpqaXdybGtKeVVFK2ox?=
 =?utf-8?B?aGJmMklTekx0b1I2eDRXUmNZVVJvNHBWaXlLUlFaR0JIaSs3QWpCVTlEcTM4?=
 =?utf-8?B?dDh4VlM4SUhoWnFWZURFZ2gybjhzVytJV0ZsSmtabmd2NUp3WHA4N3A2cDRK?=
 =?utf-8?B?ZWpvSG1BdDZDYmFVRlJ4Um9SSWJPdVFDSjhaNGZycG9YbVVoU1I1aWk4L1NX?=
 =?utf-8?B?aHBFamVoQjBZSmh5MHRxSm1wbHRaWk01TlU4SlIrcXovZUlyUDNRK1JXMlpH?=
 =?utf-8?B?Mnc3Nnd1YUdLVW0zakZ1R2hOTDlVbTU2NmVoeHhuUEZxSDdzYytjYUtSRFBS?=
 =?utf-8?B?WUpzdTBxbUhUWjNOTmlNdHkvaVA2ZkRvMkRtMjNZejdUZEovWFpjbUEreVZ6?=
 =?utf-8?B?Q2dLYjVVaHQ3b2JyS1FYcHdGNlpEOEIxY2lwTXF6RExaOFVENXBZektDTnYw?=
 =?utf-8?B?bUhEb1RwcXlqU2tiaUZXMnRxamhTV1JBUGJxdmNIQkFEQXcvRnVZRlB5N011?=
 =?utf-8?B?TVZRRDBWVEhrc0Y5d0MzVFZyNmJwUFVsNjlySWx2TUovK0FNSDZZcXF6bXZR?=
 =?utf-8?B?M1ZjZ0dJcm5ZNm8xcnV0L28xUmVuV1ErQ3huNmE0TkRsRUo1K05hNk5pQVBs?=
 =?utf-8?B?VCtURWR3U2QyamlzdVMwOWR5a3EzTWRrVmNvSGFBV0dhU05MdWZ6Zm40N0pt?=
 =?utf-8?B?a2V1d2VmVndDZFZPMUIzZ1lCYWpwWEdHcWVCenNHL011VzZYQURtVDBJSTZD?=
 =?utf-8?B?dDVjTy9BMjB5K1dmZmtON1hGRE45bHplM2RQd2pKUzhIVnRRTlhxZ1dCQ0pK?=
 =?utf-8?B?TFBQejIyZHJpdVNhSFN6cjlqYWY1dTJvcitmVWUyZHVvUUQrMGMxOU1XVE1h?=
 =?utf-8?B?VmNMZ1RJWUpQV0Z3dzBTQlhndm0xdjBKa0x0YW9GODZiYlp1all2ZmViNmd0?=
 =?utf-8?B?VHlUQVZ2S09FQ3llMGpYcnpqVm9Hck1VMFdBcDU0QVR6QU1ORlBjODlYQzZH?=
 =?utf-8?B?WmxHZUJ5V3VvWlppZWg5alR1eisrVHdGZjhxNFNOemRzNDY3d1J4ckFvNE9w?=
 =?utf-8?B?cUpDaTI3b1J2RDRQa1QyYWs3YURucndyWkRBSk9IQnFYb2k4VjRTQmtSbjFr?=
 =?utf-8?B?L3FUbXlONGo5aUsyR0Jtd0pucDRldmlTWk5qN0Fqa3JmamNFNWtNWlltVWZj?=
 =?utf-8?B?RXpzbElqVHFRZ0FrMU1VVFdTOEh6WVJPRXpGM0QvVGsxWUtMdVllZzhsUTBz?=
 =?utf-8?B?MGgxY25rcHJIK1gvQjhCaVZPZzVPVndodkhqZ2tYVHk4ZFcyZm1Jb1k2djNZ?=
 =?utf-8?B?QzJ1UEFSY3JNMjBTelRxbGxQWEZiTUlOYzJqSkk4bktMSXhDVEM2bng1b2pF?=
 =?utf-8?B?cUllVEVsbUNDa0JUS1lKNkRLYi8ybFhldHVzSnBOUXo3TEhxUTdjRDlHaHAv?=
 =?utf-8?B?VUlnakRXMnVqLzYyRjN3TUllK3pUTmw5TGp3UDBIQThYcU92cEtNbUUyNGdZ?=
 =?utf-8?B?V3hkUmFPODFqUUlXYWI1Um1kYktLeTBCSlRXaGpEM1VpY2RoeHppclUyL2Zn?=
 =?utf-8?B?ZWdERlVYcUU1WCtTL09Dc1JmTUs5QUpGdFBmQnBaNGhpY1haQnAxWDE0YzVw?=
 =?utf-8?B?RTZYeG1HUXNNRHd3YmtGZG5SU0EwNDNRckZzNytXK21YUUVxSm1RTnp5bWsv?=
 =?utf-8?B?L3lPaUlrd1E2TFpsY2R4NkNuUzY3TE12RlgwQzBVbVVqUXIwa0hsWTlEdkdT?=
 =?utf-8?B?S21yU2ZmNGN1TlAvRjBtMERuR0JHZUIrd1p6TysrWVRDNFZVS243bytoblFW?=
 =?utf-8?Q?KfxbUFYycqnUE8ENqPO7ZWo=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ed10018b-5ddf-4962-2a18-08db1f341aa7
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2023 17:48:05.1324
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3zDpeyR2RkhbExCygpt4xz7OV/etL4vTGatOs4F4cNZR4JASnduC/SXA+07DvcuFxqLVIoaNKaIt3lxYK5t1uHmV4WQ2SRbj8jufEkU5ch0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5513
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Kal Conley <kal.conley@dectris.com>
Date: Tue,  7 Mar 2023 18:23:06 +0100

> The number of chunks can overflow u32. Make sure to return -EINVAL on
> overflow.
> 
> Fixes: bbff2f321a86 ("xsk: new descriptor addressing scheme")
> Signed-off-by: Kal Conley <kal.conley@dectris.com>
> ---
>  net/xdp/xdp_umem.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index 4681e8e8ad94..f1aa79018ce8 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -150,10 +150,11 @@ static int xdp_umem_account_pages(struct xdp_umem *umem)
>  
>  static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
>  {
> -	u32 npgs_rem, chunk_size = mr->chunk_size, headroom = mr->headroom;
> +	u32 chunk_size = mr->chunk_size, headroom = mr->headroom;
>  	bool unaligned_chunks = mr->flags & XDP_UMEM_UNALIGNED_CHUNK_FLAG;
> -	u64 npgs, addr = mr->addr, size = mr->len;
> -	unsigned int chunks, chunks_rem;
> +	u64 addr = mr->addr, size = mr->len;
> +	u64 chunks, npgs;
> +	u32 chunks_rem, npgs_rem;

The RCT declaration style is messed up in the whole block. Please move
lines around, there's nothing wrong in that.

>  	int err;
>  
>  	if (chunk_size < XDP_UMEM_MIN_CHUNK_SIZE || chunk_size > PAGE_SIZE) {
> @@ -188,8 +189,8 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
>  	if (npgs > U32_MAX)
>  		return -EINVAL;
>  
> -	chunks = (unsigned int)div_u64_rem(size, chunk_size, &chunks_rem);
> -	if (chunks == 0)
> +	chunks = div_u64_rem(size, chunk_size, &chunks_rem);
> +	if (chunks == 0 || chunks > U32_MAX)

You can change the first cond to `!chunks` while at it, it's more
preferred than `== 0`.

>  		return -EINVAL;

Do you have any particular bugs that the current code leads to? Or it's
just something that might hypothetically happen?

>  
>  	if (!unaligned_chunks && chunks_rem)
> @@ -201,7 +202,7 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
>  	umem->size = size;
>  	umem->headroom = headroom;
>  	umem->chunk_size = chunk_size;
> -	umem->chunks = chunks;
> +	umem->chunks = (u32)chunks;

You already checked @chunks fits into 32 bits, so the cast can be
omitted here, it's redundant.

>  	umem->npgs = (u32)npgs;
>  	umem->pgs = NULL;
>  	umem->user = NULL;

Thanks,
Olek