Message-ID: <968526f7-d262-b69e-ca72-f56078158000@intel.com>
Date:   Wed, 8 Mar 2023 14:32:40 +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
To:     Kal Cutter 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>
 <ee0aa756-4a9c-1d7a-4179-78024e41d37e@intel.com>
 <CAHApi-kcvc2qB0D6dV7OG99FsnzAEa-rchOMfySkZ-E=EOh_4w@mail.gmail.com>
Content-Language: en-US
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <CAHApi-kcvc2qB0D6dV7OG99FsnzAEa-rchOMfySkZ-E=EOh_4w@mail.gmail.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?WE9mYUJrZFE0bm9Fd0NRSU42Sm1mcXBSRHYyUzBNL0RZL2J6VTRxQ281RVBh?=
 =?utf-8?B?aEdpQUhzWkNnZ2ppeDZ4OUwxc05URm1ibk5LNUYvZ2c1QkFDRkFGNDMzNzFq?=
 =?utf-8?B?eU9LYVJYS2N5ODNKZ2FGd3VJeHQ4dXpOMllrOXo2VHZJcE11WFB5ZitQbFZG?=
 =?utf-8?B?OEFFWWRaNFZVS3RVdTFyZCtVbkpXQjU4ckx0YWF0TjlkQXZvOWl0WjR4K25v?=
 =?utf-8?B?T3BidnNkcTJUSVdZZ3pJWXJmQjFlaDNJQjhaZG9Va0o2YTN0SWs1cmpLcWtk?=
 =?utf-8?B?ZnlDWVVpNWgwakFOUW1ENVhUM3RVaTdOUHNiM3RhekIxZ3hKdlBzNGJtdjcz?=
 =?utf-8?B?UHVvRm5mRW5yaW5kY2lyTEZmMVVxNGp5L1BBZDNYTXJZNmkwUmpoeVBMNE9p?=
 =?utf-8?B?N2dmMWF4aThyelBhQTdhd0ZpTEtsR2QxL1BnQndqaEhDZTNoRDRWYlRqNDhr?=
 =?utf-8?B?TUhLU1R1NWdFcjFiQWxsTkdxcHJFZUN6T2hBb1JSenVQaC9TbmpCdFNVMW82?=
 =?utf-8?B?VHJya2ZtVVBTZ3ZKb1BxeU4vNUxWSmtHdlpkS3Z1Smo3SHlYWmZZQkdhSW44?=
 =?utf-8?B?VnM4SWtuSUY2OWxzd1k0c2wxYWJxUmFYUldNMnlVRlFRdnJ6TWpzejJoOWpJ?=
 =?utf-8?B?bmhubC93bjZOQjVBQlZVaXlmUHJuak5KTTBXQURaMUU1Qk9iYktqcys5MVlV?=
 =?utf-8?B?Z3dOaGlzcEVyYVlXVVpDdFVYM0FFZ3B1M0p5a2tCbUszK2RLTGdHY0VQV251?=
 =?utf-8?B?aGp6eUgyNDhGNDJBZUp1bGNORXFkdGFiQ25UK3d6K3JUelJvVDBhZ3NoYkpw?=
 =?utf-8?B?cDRQZjNCanBPTjJ4TzZhb0MzK1dqQmk1SlBiZmt6aEU5aGsvVGd6cUNrTW5m?=
 =?utf-8?B?TThSbkZra0JmTDlsOU53aWJDMmExWWlYNlZRenM0K1R2NldWaXFySEhPa1Fa?=
 =?utf-8?B?cUZHek45TStBaDdLMFpRclJDSlE1MHpJTkdzWDVtby8wRndIeEtia2tIeTlO?=
 =?utf-8?B?dElvdUI0bytnWXpRZDFZMXE5VTBJZU5MdFZMRTJNekJhaVIwcm9sWllvcVB5?=
 =?utf-8?B?RW9TNU5GaWRzNW85QWZYeU90dWJIZTlLNGRaUUlIWGtsZnY2S3FkbEhuNzkw?=
 =?utf-8?B?TENzb3B5QjY5ZTFEa2VJVG9pTG5zNnl3V1ZVYnE0NVBTb3l4aE9EcmlTNTFh?=
 =?utf-8?B?Y3NVNkFkanRkb2Ewek91cnN5S2x3M2txS293U2hqckZ2M2U4VXA1YzNJWllQ?=
 =?utf-8?B?NXJDUk1SRDA4VUZEbE1SeWdmZkd2c09YNWJKSnM5TENaUEZwR3kxNUt6UUlD?=
 =?utf-8?B?YzZ4UmlJdlJEa0FseWZlSmRQNnU2ZDlnNjNmRmNJL2I5OXJQYm96UXFxUCtV?=
 =?utf-8?B?R0ZBcmU3TElWQnhkM2dLenA4aFdNM1Vwc2JYcUZKTGtEUzRjOWhZRWR6RDhD?=
 =?utf-8?B?VUJZZHVxNzlaYzJwSkpmbGFUdlFucFFxWm5xbWRLRWRMS1FxVEFWVUdCbTQz?=
 =?utf-8?B?MEo2UGVaZ0plUnpub0FtME9JUUdkOHN2ajJnZVFBSnZlUUZtOWhuSkZ4TmQ5?=
 =?utf-8?B?Ui9TNlBJaDNYV215UEEwUVFoeS8rbjk3TXJCYkN2Uk5yMFdReWZxQ2ZMSkxV?=
 =?utf-8?B?dDFkZ0o4M3ZmQUkwS2dHTTVnQ1JkaTJqZERmWlVFSElKd3NCZ0lRMWY3dUxC?=
 =?utf-8?B?ek5aYThidmRTYXBtUDE2bW5RdzVZM3NOdUZlaEVNVXpkNmI2NGNlR3pSclA1?=
 =?utf-8?B?R3hHaTNJSEJraFlONW1VN3VKSi95ZmxDQWJCQ1hpK0YvS29pdjQ4dVh0QnFh?=
 =?utf-8?B?WGdzZXFlZ0dieThYYXYrb1VXRk1DMnFZaGh6N0VxK1ZXNzJML3pjdlJlUXU0?=
 =?utf-8?B?WWErQUNpK2tXOFlyNnpPVFk3VU9VV1lrTUpJaUkwZXVSNUtNdjRaVU8rV0pJ?=
 =?utf-8?B?a3VNZGZEYU42ZXFuTW9kM3NwS1NNNE9rRDdHZnBGZ3hDZlVodGs4ZkpWRjNu?=
 =?utf-8?B?b3EyUDJkS3M5TVpNTEFuNm44bC81b3RybmNUZW5pWHdLcXFVZElQaEpBRHBk?=
 =?utf-8?B?b2xUL3cwa3dnSWNKQjdlT1EzMUdFK2hwQ3I5STVmSHVPSllKQi9CUzZ1WERt?=
 =?utf-8?B?T3JwemlaajNaSkZKaDYrQzBGbTBrYlVRU2dSV25RZHRIaHlSWUNYMWZYTkRD?=
 =?utf-8?Q?ecjINnwdjNLMd918/q6FQt0=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 46e36df1-a0b4-46d2-bd63-08db1fd9b96f
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Mar 2023 13:33:38.5341
 (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: 0E+35uxSa1X0R+Cl1dtSfy73kPURFihWc2GhZ5BT77OIg4nPcgXzc8BbokwaHrHhnMIGIfmlGljK/b6YthEETSEB7+MQh0fSG6LYo7i8DCA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7097
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 19:58:51 +0100

>> The RCT declaration style is messed up in the whole block. Please move
>> lines around, there's nothing wrong in that.
> 
> I think I figured out what this is. Is this preference documented
> somewhere? I will fix it.

It's when you sort the declarations by the line length. I.e.

short var a;
longest var b;
medium var c;

=>

longest var b;
medium var c;
short var a;

I think it's documented somewhere in the kernel. You can try grepping by
"Reverse Christmas Tree".

> 
>>
>>>       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`.
> 
> If you want, I can change it. I generally like to keep unrelated
> changes to a minimum.

You modify the line either way, so I don't see any reasons to keep the
code as-is. It's clear that replacing `== 0` to `!chunks` won't change
the logic anyhow.

> 
>>
>>>               return -EINVAL;
>>
>> Do you have any particular bugs that the current code leads to? Or it's
>> just something that might hypothetically happen?
> 
> If the UMEM is large enough, the code is broke. Maybe it can be
> exploited somehow? It should be checked for exactly the same reasons
> as `npgs` right above it.
> 
>>
>>>
>>>       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.
> 
> I made it consistent with the line right below it. It seems like the
> cast may improve readability since it makes it known the truncation is
> on purpose. I don't see how that is redundant with the safety check.
> Should I change both lines?

I'd prefer to change both lines. You already check both @npgs and
@chunks for being <= %U32_MAX and anyone can see it from the code, so
the casts don't make anything more readable.

> 
>>
>>>       umem->npgs = (u32)npgs;
>>>       umem->pgs = NULL;
>>>       umem->user = NULL;
>>
>> Thanks,
>> Olek
> 
> Kal

Thanks,
Olek