Re: [PATCH v5 4/5] crypto: caam - add in-kernel interface for blob generator

From: Ahmad Fatoum <a.fatoum@pengutronix.de>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: "Horia Geantă" <horia.geanta@nxp.com>,
	"Aymen Sghaier" <aymen.sghaier@nxp.com>,
	"Herbert Xu" <herbert@gondor.apana.org.au>,
	"David S. Miller" <davem@davemloft.net>,
	kernel@pengutronix.de, "David Gstir" <david@sigma-star.at>,
	"Pankaj Gupta" <pankaj.gupta@nxp.com>,
	"Tim Harvey" <tharvey@gateworks.com>,
	"Matthias Schiffer" <matthias.schiffer@ew.tq-group.com>,
	"James Bottomley" <jejb@linux.ibm.com>,
	"Mimi Zohar" <zohar@linux.ibm.com>,
	"David Howells" <dhowells@redhat.com>,
	"James Morris" <jmorris@namei.org>,
	"Eric Biggers" <ebiggers@kernel.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	"Jan Luebbe" <j.luebbe@pengutronix.de>,
	"Richard Weinberger" <richard@nod.at>,
	"Franck LENORMAND" <franck.lenormand@nxp.com>,
	"Sumit Garg" <sumit.garg@linaro.org>,
	linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
	linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org
Subject: Re: [PATCH v5 4/5] crypto: caam - add in-kernel interface for blob generator
Date: Wed, 16 Mar 2022 17:44:08 +0100	[thread overview]
Message-ID: <fc13e731-a419-cad6-02f3-44fb25bee6a1@pengutronix.de> (raw)
In-Reply-To: <79b912b5e9e16f446753270f7b9463fee95ebac7.camel@kernel.org>

Hello Jarkko,

On 28.02.22 13:14, Jarkko Sakkinen wrote:
> On Wed, 2022-02-23 at 17:20 +0100, Ahmad Fatoum wrote:
>> On 23.02.22 16:41, Jarkko Sakkinen wrote:
>>> On Tue, Feb 22, 2022 at 08:58:18PM +0100, Ahmad Fatoum wrote:
>>>> +       /* header + (key mod immediate) + 2x pointers + op */
>>>> +       len = 4 + (4 + ALIGN(keymod_len, 4)) + 2*(4 + 4 + CAAM_PTR_SZ_MAX) + 4;
>>>
>>> Nit: the amount of magic numbers is overwhelming here. I neither understand
>>> the statement nor how that comment should help me to understand it.
>>
>> header              =  4
>> (key mod immediate) = (4 + ALIGN(keymod_len, 4))
>> 2x pointer          =  2 * (4 + 4 + CAAM_PTR_SZ_MAX)
>> op                  =  4
> 
> Please create a struct with the associated fields instead and then
> it is just sizeof that.

The CAAM descriptor construction code writes the current length
to the first word and then macros are used to append fields to
the descriptor while incrementing the length. The key modifier
is in the middle of the descriptor, so this isn't representable
as a C struct without using VLAs, which are banned in the kernel.
Even if it worked, it's arguably not good style to define a struct
just to compute the size and then cast it to a u32 * to use the
same macros all other CAAM code uses.

I thought this over a bit and figured that we don't really need to
compute the length dynamically as it's at most 44 or 52 bytes
(depending on DMA address width) anyway, so we can just use a fixed
value. I still keep the addition, because I believe there is a value
in seeing how we arrive at the 44 or 52 bytes instead of just hardcoding
it.

Just sent out v6 with these changes. Let me know what you think.

Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |