p[ On Mon, 21 Oct 2019 at 17:23, Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx> wrote: > > > -----Original Message----- > > From: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > > Sent: Monday, October 21, 2019 2:54 PM > > To: Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx> > > Cc: linux-crypto@xxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx > > Subject: Re: Key endianness? > > > > On Mon, 21 Oct 2019 at 14:40, Pascal Van Leeuwen > > <pvanleeuwen@xxxxxxxxxxxxxx> wrote: > > > > > > > -----Original Message----- > > > > From: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > > > > Sent: Monday, October 21, 2019 1:59 PM > > > > To: Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx> > > > > Cc: linux-crypto@xxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx > > > > Subject: Re: Key endianness? > > > > > > > > On Mon, 21 Oct 2019 at 12:56, Pascal Van Leeuwen > > > > <pvanleeuwen@xxxxxxxxxxxxxx> wrote: > > > > > > > > > > Another endianness question: > > > > > > > > > > I have some data structure that can be either little or big endian, > > > > > depending on the exact use case. Currently, I have it defined as u32. > > > > > This causes sparse errors when accessing it using cpu_to_Xe32() and > > > > > Xe32_to_cpu(). > > > > > > > > > > Now, for the big endian case, I could use htonl()/ntohl() instead, > > > > > but this is inconsistent with all other endian conversions in the > > > > > driver ... and there's no little endian alternative I'm aware of. > > > > > So I don't really like that approach. > > > > > > > > > > Alternatively, I could define a union of both a big and little > > > > > endian version of the data but that would require touching a lot > > > > > of legacy code (unless I use a C11 anonymous union ... not sure > > > > > if that would be allowed?) and IMHO is a bit silly. > > > > > > > > > > Is there some way of telling sparse to _not_ check for "correct" > > > > > use of these functions for a certain variable? > > > > > > > > > > > > > > > > > In this case, just use (__force __Xe32*) to cast it to the correct > > > > type. This annotates the cast as being intentionally endian-unclean, > > > > and shuts up Sparse. > > > > > > > Thanks for trying to help out, but that just gives me an > > > "error: not an lvalue" from both sparse and GCC. > > > But I'm probably doing it wrong somehow ... > > > > > > > It depends on what you are casting. But doing something like > > > > u32 l = ... > > __le32 ll = (__force __le32)l > > > > should not trigger a sparse warning. > > > I was actually casting the left side, not the right side, > as that's where my sparse issue was. Must be my poor grasp > of the C language hurting me here as I don't understand why > I'm not allowed to cast an array element to a different type > of the _same size_ ... > > i.e. why can't I do (__be32)some_u32_array[3] = cpu_to_be32(some_value)? > Because you can only change the type of an expression by casting, and an lvalue is not an expression. A variable has a type already, and you cannot cast that away - what would that mean, exactly? Would all occurrences of some_u32_array[] suddenly have a different type? Or only element [3]? > I managed to work around it by doing *(__be32 *)&some_u32_array[3] = > but that's pretty ugly ... a better approach is still welcome. > You need to cast the right hand side, not the left hand side. If some_u32_array is u32[], force cast it to (__force u32) > > > > > > > Regards, > > > > > Pascal van Leeuwen > > > > > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix > > > > > www.insidesecure.com > > > > > > > > > > > -----Original Message----- > > > > > > From: Pascal Van Leeuwen > > > > > > Sent: Monday, October 21, 2019 11:04 AM > > > > > > To: linux-crypto@xxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx > > > > > > Subject: Key endianness? > > > > > > > > > > > > Herbert, > > > > > > > > > > > > I'm currently busy fixing some endianness related sparse errors reported > > > > > > by this kbuild test robot and this triggered my to rethink some endian > > > > > > conversion being done in the inside-secure driver. > > > > > > > > > > > > I actually wonder what the endianness is of the input key data, e.g. the > > > > > > "u8 *key" parameter to the setkey function. > > > > > > > > > > > > I also wonder what the endianness is of the key data in a structure > > > > > > like "crypto_aes_ctx", as filled in by the aes_expandkey function. > > > > > > > > > > > > > > crypto_aes_ctx uses CPU endianness for the round keys. > > > > > > > So these will need to be consistently handled using cpu_to_Xe32. > > > > > > > If you are using the generic aes_expandkey and want to reuse the key > > schedule, it is indeed good to be aware that both the round keys > > themselves as well as the key length are recorded in CPU endianness. > > > Actually, I have a big patch standing by getting rid of aes_expandkey() > altogether as I don't need _any_ of those round keys generated, ii > was just used for AES key validity checks and nothing else. > > But since that patch is not ready for prime time yet, I have to fix > these sparse errors for the time being. > > > > > In general, though, there is no such thing as endianness for a key > > > > that is declared as u8[], it is simply a sequence of bytes. > > > > > > > Depends a bit on the algorithm. Some keys are indeed defined as byte > > > streams, in which case you have a point. Assuming you mean that the > > > crypto API follows the byte order as defined by the algorithm spec. > > > > > > But sometimes the key data is actually a stream of _words_ (example: > > > Chacha20) and then endianness _does_ matter. Same thing applies to > > > things like nonces and initial counter values BTW. > > > > > > > Endianness always matters, and both AES and ChaCha are rather similar > > in that respect in the sense that it is the algorithm that defines how > > a byte stream is mapped onto 32-bit words, and in both cases, they use > > little endianness. > > > Thanks, that's actually something I can _use_ ;-) > > > > > > > If the > > > > hardware chooses to reorder those bytes for some reason, it is the > > > > responsibility of the driver to take care of that from the CPU side. > > > > > > > Which still requires you to know the byte order as used by the API. > > > > > > > Only if API means the AES or ChaCha specific helper routines that we > > have in the kernel. If you are using the AES helpers, then yes, you > > need to ensure that you use the same convention. But the algorithms > > themselves are fully defined by their specification, and so what other > > implementations in the kernel do is not really relevant. > > > What is relevant is what the API expects But *which* API? The skcipher API uses u8[] for in/output and keys, and how these byte arrays are interpreted is not (and cannot) be defined at this level of abstraction. > ... and from 20 years of > experience I would say many algorithm specifications are not exactly > very clear on the byte order at all, often assuming this to be > "obvious". (and if it's not little-endian, it's not obvious to me ;-) > I agree that not all specs are crystal clear on this. But it is still the algorithm that needs to define this. > Very often getting the byte order right was just trial and error > using known-good reference vectors and just trying every possible > byte/word/whatever swap you could think of. (hence "ellendianness") > > > > > > > > > > > > > > > Since I know my current endianness conversions work on a little endian > > > > > > CPU, I guess the big question is whether the byte order of this key > > > > > > data is _CPU byte order_ or always some _fixed byte order_ (e.g. as per > > > > > > algorithm specification). > > > > > > > > > > > > I know I have some customers using big-endian CPU's, so I do care, but > > > > > > I unfortunately don't have any platform available to test this with. > > > > > > > > > > > > > > You can boot big endian kernels on MacchiatoBin, in case that helps > > > > (using u-boot, not EFI) > > > > > > > I'm sure _someone_ can, I'm not so sure _I_ can ;-) > > > > > > Regards, > > > Pascal van Leeuwen > > > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix > > > www.insidesecure.com > > > Regards, > Pascal van Leeuwen > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix > www.insidesecure.com >
