> -----Original Message----- > From: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > Sent: Monday, October 21, 2019 5:32 PM > To: Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx> > Cc: linux-crypto@xxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx > Subject: Re: Key endianness? > > 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 think it would be perfectly logical to do such a cast and I'm really surprised that it is not legal. Obviously, it would only apply to the actual assignment it is used with. It's a cast, not a redefinition. After all, a variable or an array item is just some storage area in memory. Why shouldn't I be able to write to it _as if_ it is some different type (if I know what I'm doing and especially if it is the exact same size in memory)? > > > 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) > Sure, you can do the casting on the right hand side, but that may not convey what you _really_ want to do, particularly in this case. As I _really_ want to write a big endian word there. I don't want to pretend I loose the endianness somewhere along the way. That written word is still very much big endian. (I know practically it makes no difference, but casting the left side would just be so much clearer IMHO) > > > > > > > > > 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. > Yes, skcipher API. Obviously. As that's what we're talking about. And _of course_ it has to be defined at that level of abstraction. (which doesn't preclude inheriting it from some other specification) Otherwise you would not able to e.g. exchange keys between different platforms. > > > ... 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. > In an ideal world, probably. In the real world, it is entirely possible for an implementation to expect the key bytes in a different order. Would not be the first time I run into that. > > 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 > > Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com
