Hi Max,

On Sat, Dec 28, 2013 at 10:02:10PM +0100, ☎ wrote:

Improved version of 1st patch (rol16 implementation were missing) and actual kasumi implementation which uses macro from 1st patch.

thanks for your patch.

I think it would be better if the load/store macros would at least have an OSMO_ name prefix in order to avoid namespace clashes with other code. I'd consider that mandatory for a merge.

As a matter of style, I would prefer to have proper inline functions rather than hard to read multi-line macros. I'd consider that optional, as we could still increase readibility later on by either breaking the macros into multi-line macros or inline functions at a later point without breaking the API / uesers.

What you get from inline functions is better compiler warnings in case of type mismatches on the input side. Trying to understand compiler warnings in complex macros is much harder.

Regards, Harald

On Tue, Jan 14, 2014 at 01:19:03PM +0100, ☎ wrote:

Hi.

Attached is v2: converted to inline functions, fixed all compiler warnings and changed names to osmo_*

Note: there are not much tests but kasumi and msgb code uses store/load routines so it could be considered as an indirect test case.

tests is some how important. At least knowing that all routines got executed at least once.

+static inline uint16_t osmo_load16le(const uint8_t *p) +{//Load unaligned 16-bit integer (little-endian encoding)

C89 didn't have this style of comments. Use /* */ but maybe consider using api comments above the function instead of directly after {?

+static inline uint16_t osmo_load16be(const uint8_t *p) +{//Load unaligned 16-bit integer (big-endian encoding)

• return ((((uint16_t *)(p))[0] << 8) | ((uint16_t *)(p))[1]);

this is not lisp. Can you remove the number of braces? Now that this is a method you can omit the () around p. In terms of readability.

+_kasumi; +_kasumi_key_expand; +_kasumi_kgcore;

too early. please move that to the later patch.

cheers holger

I've converted my comments but I'd like to point out that libosmocore does not actually follow this style - just do git grep -n "//"|grep -v http to verify.

There is a _LOT_ less // than /* */ style comments.

bash$ git grep -n "/*"|grep -v http | grep -v git | grep -v \.in | grep -v rules | wc -l 4859 bash$ git grep -n "//"|grep -v http | grep -v git | grep -v \.in | grep -v rules | wc -l 26

Just because a few slipped by the review in previous merges doesn't mean it should be encouraged.

Also not that among those few, only 2 of them are actually comments really, the others are deactivated code that really should have been removed instead of commented.

Cheers,

Sylvain

Thanks for comment, I'll try to address them in next version. Could you comment on the 2nd patch as well?

05.02.2014 10:00, Holger Hans Peter Freyther пишет:

On Fri, Jan 31, 2014 at 03:10:48PM +0100, ☎ wrote:

...

v3 attached, comments are inline.

...

...

this is not lisp.

Yepp, that makes me sad too :) Anyway, fixed.

hmm.. another lisp comment.

...

+/* Load unaligned 16-bit integer (little-endian encoding) */ +static inline uint16_t osmo_load16le(const uint8_t *p) +{

• return ((uint16_t)p[0]) | ((uint16_t)p[1] << 8);

+}

do you really need to cast both of them to uint16_t? isn't the result enough?

...

+/* Store unaligned 16-bit integer (little-endian encoding) */ +static inline void osmo_store16le(uint16_t a, uint8_t *p) +{

• ((uint8_t *)p)[0] = (a) & 0xFF;
• ((uint8_t *)p)[1] = (a >> 8) & 0xFF;

+}

these and the below uint8_t casts are not needed for sure. p is already a uint8_t *. No need to write it down?

sorry. I try to integrate v4 more quickly. :}

And v5:

- switched to code generation similar to crcXX: no more lisp-ness :) - expanded test-suites

that was fun - the template surely looks much less lispier (and more ugly IMO) but it's covered by at least 3 test suits: bits (full), msgb and kasumi (partial) so I'm pretty sure it works.

05.02.2014 20:55, Holger Hans Peter Freyther пишет:

On Wed, Feb 05, 2014 at 08:13:20PM +0100, ☎ wrote:

...

And v5:

• switched to code generation similar to crcXX: no more lisp-ness :)
• expanded test-suites

that was fun - the template surely looks much less lispier (and more ugly IMO) but it's covered by at least 3 test suits: bits (full), msgb and kasumi (partial) so I'm pretty sure it works.

No more msgb.h change? Did I miss it?

Doh!

I missed it somehow- it's exactly the same as in all the previous versions. I'll make another iteration.

On Wed, Feb 05, 2014 at 09:09:37PM +0100, ☎ wrote:

+/* Store unaligned n-byte integer (little-endian encoding) into uintXX_t */ +static inline void osmo_storeXXle_ext(uintXX_t x, uint8_t *p, uint8_t n) +{

• uint8_t i;
• for(i = 0; i < n; p[i] = (x >> i * 8) & 0xFF, i++);

+/*

• uint8_t i, adj = 8 * (8 - n);
• uintXX_t y = (x << adj) >> adj;
• for(i = 0; i < n; p[i] = (y >> i * 8) & 0xFF, i++);

+*/

left over comments or is that expanded in another way?

+/* Store unaligned n-byte integer (big-endian encoding) into uintXX_t */ +static inline void osmo_storeXXbe_ext(uintXX_t x, uint8_t *p, uint8_t n) +{ +/*

• uint8_t i, adj = 8 * (8 - n);
• uintXX_t y = (x << adj) >> adj;
• for(i = 0; i < n; p[i] = (y >> ((n - 1 - i) * 8)) & 0xFF, i++);

+*/

left over?

+/*

• Less trivial LE/BE functions are autogenerated
• see included bitXXgen.h files

+*/

What makes them more trivial?

diff --git a/tests/bits/bitrev_test.c b/tests/bits/bitrev_test.c index 5eca990..6cf340e 100644 --- a/tests/bits/bitrev_test.c +++ b/tests/bits/bitrev_test.c

+char s[18], *p;

make static?

+void check_ls_64(uint8_t bytes)

static too

+{

• uint8_t T[bytes], D = (8 - bytes), A = 8 * D, C = 2 * D;
• uint64_t _test = ((uint64_t)rand() << 32) + rand(), a_test = (_test << A) >> A;

that looks a bit crazy? What is your goal? Can you simplify it?

• if (0 != memcmp(s + C, p, bytes)) {

• printf("%s\t%s\t%u BE store FAILED!\n", s + C, p, bytes * 8);
  

• } else printf("%u BE store OK\n", bytes * 8);

coding style. :)

• if (0 != memcmp(s, p, 2)) {

• printf ("%s\t", s);
  

• printf ("%s\t", p);
  

• printf("16 BE FAILED on %" PRIx16 "\n");
  

• } else printf("16 BE store OK\n");

coding style. :)

From ab98c898fa54e441fdb0b08145bad074c09f2cf1 Mon Sep 17 00:00:00 2001 From: Max max.suraev@fairwaves.ru Date: Wed, 5 Feb 2014 20:10:38 +0100 Subject: [PATCH 2/2] Add Kasumi cipher implementation

•                   osmocom/core/bit32gen.h \
  

•                   osmocom/core/bit64gen.h \
  

that is is in the wrong patch?

+osmocom/core/bit%gen.h: osmocom/core/bitXXgen.h.tpl

• $(AM_V_GEN)$(MKDIR_P) $(dir $@)
• $(AM_V_GEN)sed -e's/XX/$*/g' $< > $@

that is in the wrong patch too?

+#ifndef __KASUMI_H__ +#define __KASUMI_H__

_/__ are reserved for the system. I started to use "#pragma once"

+uint64_t _kasumi(uint64_t P, uint16_t *KLi1, uint16_t *KLi2, uint16_t *KOi1, uint16_t *KOi2, uint16_t *KOi3, uint16_t *KIi1, uint16_t *KIi2, uint16_t *KIi3);

can you mark some of the parameters as const?

+static uint16_t +_kasumi_FI(uint16_t I, uint16_t skey)

coding style. :)

• static uint16_t S7[] = {

Mark these arrays as const too. GCC is clever enough to figure out it is static.

• static uint16_t S9[] = {

Same here.

• L ^= (skey & 0x1FF);
• R ^= (skey >> 9);

Extra spaces?

• L = S9[L] ^ R;
• R = S7[R] ^ (L & 0x7F);

Extra spaces?

+static uint32_t +_kasumi_FO(uint32_t I, uint16_t *KOi1, uint16_t *KOi2, uint16_t *KOi3, uint16_t *KIi1, uint16_t *KIi2, uint16_t *KIi3, unsigned i)

coding style. :)

+static uint32_t +_kasumi_FL(uint32_t I, uint16_t *KLi1, uint16_t *KLi2, unsigned i)

coding style. :)

+uint64_t +_kasumi(uint64_t P, uint16_t *KLi1, uint16_t *KLi2, uint16_t *KOi1, uint16_t *KOi2, uint16_t *KOi3, uint16_t *KIi1, uint16_t *KIi2, uint16_t *KIi3)

coding style. :)

+/*! \brief Expand key into set of subkeys

• • \param[in] key (128 bits) as array of bytes
• • \param[out] arrays of round-specific subkeys - see TS 135 202 for details
• */

+void +_kasumi_key_expand(const uint8_t *key, uint16_t *KLi1, uint16_t *KLi2, uint16_t *KOi1, uint16_t *KOi2, uint16_t *KOi3, uint16_t *KIi1, uint16_t *KIi2, uint16_t *KIi3)

commented again? make const?

• for (i = 0; i < 8; i++) /* Work with 16 bit subkeys and create prime subkeys */
• {

• C[i] ^= osmo_load16be(key + i * 2);
  

• }

coding style. :)

• /* C[] now stores K-prime[] */
• for (i = 0; i < 8; i++) /* Create round-specific subkeys */
• {

coding style. :)

+void +_kasumi_kgcore(uint8_t CA, uint8_t cb, uint32_t cc, uint8_t cd, const uint8_t *ck, uint8_t *co, uint16_t cl)

coding style. :)

+inline int _compare_mem(uint8_t * x, uint8_t * y, size_t len) {

• if (0 != memcmp(x, y, len)) {

static coding style (tabs vs. space)

On Thu, Mar 06, 2014 at 11:52:10AM +0100, ☎ wrote:

Thanks for comments - all either fixed or comments added to clarify.

let me have another look. On general comment. The amount of parameters you pass to the method are a lot. Would you be able to be woken up in the middle of the day/night (whatever is less comfortable) and would you know the parameters? Did you consider passing them as a struct?

+static void check_ls_64(uint8_t bytes) +{

• /* calculate various adjustment constants (number of bits, bytes, octets etc.)

• based on number of bytes in type we actually test */
  

I was more thinking about what exactly do you want to test? Encode/Decode being compatible with each other? Corner cases? This is very difficult to understand code and I don't see your intend. I wonder/guess that there is a more simple approach to it.

int main(int argc, char **argv) {

• uint8_t out[ARRAY_SIZE(input)];

• uint8_t out[ARRAY_SIZE(input)], test[8]; unsigned int offs;

• for (offs = 0; offs < sizeof(out); offs++) {

• srand(time(NULL));
• for (offs = 0; offs < sizeof(out); offs++)
• {

coding style. :)

• if (memcmp(start, exp_out + offs, len)) {
  

• if (memcmp(start, exp_out + offs, len))
  

• {
  

coding style. :)

• if (0 != memcmp(s, p, 2))
• {

coding style. :)

+void _kasumi_kgcore(uint8_t CA, uint8_t cb, uint32_t cc, uint8_t cd, const uint8_t *ck, uint8_t *co, uint16_t cl);

sorry. I didn't see that the first time. _NAME is reversed by the system.

+inline static uint16_t _kasumi_FI(uint16_t I, uint16_t skey)

_kasumi is reserved for gcc/glibc. :)

• static const uint16_t S7[] = {
• };
• static const uint16_t S9[] = {
• };

these tables were copied from the spec?

+inline static uint32_t _kasumi_FO(uint32_t I, const uint16_t *KOi1, const uint16_t *KOi2, const uint16_t *KOi3, const uint16_t *KIi1, const uint16_t *KIi2, const uint16_t *KIi3, unsigned i) +inline static uint32_t _kasumi_FL(uint32_t I, const uint16_t *KLi1, const uint16_t *KLi2, unsigned i)

again reserved symbol. :)

+uint64_t _kasumi(uint64_t P, const uint16_t *KLi1, const uint16_t *KLi2, const uint16_t *KOi1, const uint16_t *KOi2, const uint16_t *KOi3, const uint16_t *KIi1, const uint16_t *KIi2, const uint16_t *KIi3) +{

• uint32_t i, L = P >> 32, R = P; /* Split 64 bit input into Left and Right parts */
• for (i = 0; i < 8; i++)
• {

coding style. :)

+void _kasumi_key_expand(const uint8_t *key, uint16_t *KLi1, uint16_t *KLi2, uint16_t *KOi1, uint16_t *KOi2, uint16_t *KOi3, uint16_t *KIi1, uint16_t *KIi2, uint16_t *KIi3) +{

• for (i = 0; i < 8; i++)
• {

coding style. :) (and reserved symbol)

• _kasumi_key_expand(ck, KLi1, KLi2, KOi1, KOi2, KOi3, KIi1, KIi2, KIi3);
• for (i = 0; i < cl / 64 + 1; i++) /* i is a block counter */
• {

coding style. :)

time is up. probably more coding style and symbol issues. :)

On Thu, Mar 06, 2014 at 01:13:35PM +0100, ☎ wrote:

That's why I referenced corresponding standard in the comment. I do not see how adding struct will help with remembering parameter names at night - I think it will increase code complexity and decrease readability. Besides those are parameters to internal-only functions - you are not supposed to work with this code without reading corresponding standard beforehand.

Maybe you don't remember but you wil get the order right. Why do you want/ need to export the methods then? :)

...

I was more thinking about what exactly do you want to test? Encode/Decode being compatible with each other? Corner cases? This is very difficult to understand code and I don't see your intend. I wonder/guess that there is a more simple approach to it.

I'll add more comments to clarify - in general, I do not see better approach for serialize/deserialize kind of functions than trying to read/write and compare the results.

Max, what do you want to test?

Would you mind to be more specific? I know that it's linux-kernel style but it would greatly help if you spend 3 more seconds to add few words to clarify what exactly you're unhappy about - for example in this particular case I've been confused by the undescriptive comments from previous email.

if (foo) {

and not if (foo)\n{

Is there some general way to mark function as internal-use only? For example the only reason I'm exposing _kasumi* is to be able to use them in test/kasumi_test code.

ANSI C. E.g. in C99 Chapter 7.1.3 Reserved identifiers: "All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use."

Okay _kasumi is actually fine, __kasumi would not, _KASUMI would be neither. My take is that it is best to avoid anything close to reserved symbols. :)

...

these tables were copied from the spec?

Yes, sure. The test vectors in kasumi_test were taken from the spec as well.

do you want to add another reference?

On Thu, Mar 06, 2014 at 06:27:38PM +0100, ☎ wrote:

Dear Max,

...

...

I'll add more comments to clarify - in general, I do not see better approach for serialize/deserialize kind of functions than trying to read/write and compare the results.

Max, what do you want to test?

Commented.

That was not the point. I tried to ask you what you intend to test. E.g. if you take a look at Jacob's range encoding tests in OpenBSC. There is a simple list of fixes values that are tested and then some random numbers.

Now I find your testcase very hard to comprehend. This is why I want to understand the goal first and then propose a more simple/straight forward approach. But now I did propose something. ;)

You are still having for (...) but we have for (...) { {

sorry about that. But we still have some time until Sunday. :)