Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Ruhl, Michael J" <michael.j.ruhl@intel.com>
To:     Kees Cook <keescook@chromium.org>
CC:     "Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        "Eric Dumazet" <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "intel-wired-lan@lists.osuosl.org" <intel-wired-lan@lists.osuosl.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-hardening@vger.kernel.org" <linux-hardening@vger.kernel.org>
Subject: RE: [PATCH v3 2/2] igb: Proactively round up to kmalloc bucket size
Thread-Topic: [PATCH v3 2/2] igb: Proactively round up to kmalloc bucket size
Thread-Index: AQHY60UEuvCQ5/6nqkOEi2jDwjCFAa4o+uNA
Date:   Mon, 31 Oct 2022 20:42:36 +0000
Message-ID: <DM5PR11MB1324802F3F2098CB3239CF36C1379@DM5PR11MB1324.namprd11.prod.outlook.com>
References: <20221018092340.never.556-kees@kernel.org>
 <20221018092526.4035344-2-keescook@chromium.org>
 <202210282013.82F28AE92@keescook>
In-Reply-To: <202210282013.82F28AE92@keescook>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.6.500.17
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?FKTYgNaaEsiphb5sztbkXowSiWl7kv2Jq7vA7jys1ps9G4BQrez0RLtdaSXe?=
 =?us-ascii?Q?+5Fk+9U0OMIixbueS68Brr/1yxcM6MOkeUlZORPYh0Fe1nbORraPL45pkGZ7?=
 =?us-ascii?Q?3Qo9+E2I3Ck9RrQusy1rBLqDjDmVveG+EneBjTYye8uJRuIyLcV4BR81kOs0?=
 =?us-ascii?Q?58icYiAAXlQxuL92k8/hdHxNy1Nh4Fmvi4dVzIRIU9ZnHgE8ufCJlO4ghB7I?=
 =?us-ascii?Q?5K0Hool1BRbzRJ2YB74lj4ZR5Ihguw5GQOiiSlyPOzDOsx7rjhwltwsizWbn?=
 =?us-ascii?Q?ydoydOsy2NXf6IxE1jmVn+C7G+IahEEVyalqpM9OKPT76sxVKZnhFkAkq3e7?=
 =?us-ascii?Q?26VY6jAMJac/vrzk1CgPUHjb7lioZ+Z/Bk0B7fA4Vh2iqRLBkcb2G+UdfJ6W?=
 =?us-ascii?Q?UmaIihQR3tbfkjIPtUxMcPJLTrC40pljegBznzG0U9lUM2zNmnCw/sVRH0Ak?=
 =?us-ascii?Q?dPqi0Unz6xk7Xjh8W3PTtWV13cIwvHtwSWdyLDf3nK9Pd8fJJCDs6lgV5Xx/?=
 =?us-ascii?Q?8Fj8Z9SwFL0y0Ik8E8e8WjuVGClavjqqZp6U3/5oYZ2JORMdgDqTHl9UV6PN?=
 =?us-ascii?Q?8Pth90BsckzC+cMePu6jsGwO3fQnUMQ2ipCKEnsjbRt7F1ZKO3Cd3+j/jrJh?=
 =?us-ascii?Q?shwarUK7UfVNpOKMkZOCMjlCNxhbeGzLK2GsmP4I1Gq+oY4g9N7A2gErBsll?=
 =?us-ascii?Q?RTZeg8ZsjSp4xLy6SC1xl5UoFzO8an+vohyfrIx3ZhMGNScsKCxRzbunRXBC?=
 =?us-ascii?Q?d2zFbUqP94QHBjarq1c3oac4DKHyI9LEDxbJpUuyo54eRb+p3XXu6aYTufbb?=
 =?us-ascii?Q?tgw1CAKfiqq/7y97+eTNT7wTYazHH85pUEd3+Q/FOtnx5Zwb50g+8/YJoLkA?=
 =?us-ascii?Q?us7Sek/g4M4IIU7Vsy9t9Qp2cGUulFbRpTkeOs1ay9UhXLVaggywNq/f1pOO?=
 =?us-ascii?Q?CKnw0T60i09XSVqAczLY8HYj+VkWpVkDYGmXGck7q2GmLxdsJf+9otXToCjD?=
 =?us-ascii?Q?jZdj2pjnNWfUoFoF9r3iP8mdGh4QN9YsRBvNIcbODlDEtSmu3d9d4meKkq1g?=
 =?us-ascii?Q?0jPN9rF4Ot5FtpAbaeBXZM0iQRojKX2yGeuxEifLo9CemaiZs4l18H68H3t5?=
 =?us-ascii?Q?uxSCKCipor8n8wlPeu9exqI6DUtsDJ10X3Rg2k9PhdSxve0EK5XNuDi7VQq+?=
 =?us-ascii?Q?qu74vNxhlxCxcjJRgB1vtHxJKslTaT156vWRnqJmTZ9KyJK9Rr83tRkX85nO?=
 =?us-ascii?Q?WBlNxEwoF7mF8NqpP/r3BXbfTP0KJg7skoiNopH807oQgujij2CAjQsL2ftQ?=
 =?us-ascii?Q?NlaiIqRnVnj+albZhsDA2nF6yLkURd87HpaUf0SkqU51FCqwTXReoNRUOlOU?=
 =?us-ascii?Q?5dK8vpYLcHoWXvpD0JZikKCnZrmXCqJANllLM8wwn++txEqLqgteB2RKxeRV?=
 =?us-ascii?Q?AUoJs3f2m5A4XW/UnBz4urFszvMR6iece0o0qEV8OtXYSjtdC0vjnbAw/Kik?=
 =?us-ascii?Q?etK1h9ebSbOYloqfSqcLZFH6cjSDz3x1FxbPKKz+QirLWW+wFLV9tl54H237?=
 =?us-ascii?Q?Mzfla7UcNaF4dW3ZyiSCoKWJhklkV7wwP0lIjbH5?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1324.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 22ccda69-7be9-4a51-2fba-08dabb807195
X-MS-Exchange-CrossTenant-originalarrivaltime: 31 Oct 2022 20:42:36.1532
 (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: 45vzYJEP+rbvdCZa8CnTnP4y7HqkkXfDZk9jVSB83b0oP+bbO79sxcEpc+WUwmMstkx9oYHcZFIrY8elZJ6uyzWk/gtnGkE91wtbEIdRoEE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7308
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>-----Original Message-----
>From: Kees Cook <keescook@chromium.org>
>Sent: Friday, October 28, 2022 11:18 PM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L
><anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>;
>Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
>Paolo Abeni <pabeni@redhat.com>; intel-wired-lan@lists.osuosl.org;
>netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>hardening@vger.kernel.org
>Subject: Re: [PATCH v3 2/2] igb: Proactively round up to kmalloc bucket si=
ze
>
>On Tue, Oct 18, 2022 at 02:25:25AM -0700, Kees Cook wrote:
>> In preparation for removing the "silently change allocation size"
>> users of ksize(), explicitly round up all q_vector allocations so that
>> allocations can be correctly compared to ksize().
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
>Hi! Any feedback on this part of the patch pair?
>
>> ---
>>  drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
>b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 6256855d0f62..7a3a41dc0276 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -1195,7 +1195,7 @@ static int igb_alloc_q_vector(struct igb_adapter
>*adapter,
>>  		return -ENOMEM;
>>
>>  	ring_count =3D txr_count + rxr_count;
>> -	size =3D struct_size(q_vector, ring, ring_count);
>> +	size =3D kmalloc_size_roundup(struct_size(q_vector, ring, ring_count))=
;
>>
>>  	/* allocate q_vector and rings */
>>  	q_vector =3D adapter->q_vector[v_idx];

Hi Kees,

Looking at the size usage (from elixir), I see:

--
	if (!q_vector) {
		q_vector =3D kzalloc(size, GFP_KERNEL);
	} else if (size > ksize(q_vector)) {
		kfree_rcu(q_vector, rcu);
		q_vector =3D kzalloc(size, GFP_KERNEL);
	} else {
		memset(q_vector, 0, size);
	}
--

If the size is rounded up, will the (size > ksize()) check ever be true?

I.e. have you eliminated this check (and maybe getting rid of the need for =
first patch?)?

Thanks,

Mike


>
>Thanks! :)
>
>-Kees
>
>--
>Kees Cook