Hello.
Attached is A5/3,4 GEA3,4 implementation which was described today at OsmoDevCon.
This is "from the scratch" implementation, tests using test vectors from relevant 3GPP standards are included.
There are couple of bits missing: * gprs_auth API needs to be changed to use GEA4 with 128 bits key * gea_test only checks algorithm correctness but not loading/using it via gprs_auth plugin api * osmo_a5_1 and osmo_a5_2 probably should be hidden from public api - the only way to call them should be through osmo_a5() function
Please review and merge if possible.
Hi Max,
Attached is A5/3,4 GEA3,4 implementation which was described today at OsmoDevCon.
Here's some comments on the first patch. (It's late and I'm tired, I'll look at the rest later :p)
1) Why do you need to add stddef.h ? Also before of spacing change, you're removing an empty line that was there to separate the includes from the function comment.
2) wrt to :
+uint16_t osmo_get2bytes(const uint8_t *a); +void osmo_64pack2pbit(uint64_t in, pbit_t *out);
Theses essentially look like integer accessors, one is a read for uint16_t in big endian and the other a store for uint64_t in little endian, but same basic idea. But you're using completely different naming schemes for both ... I think they should reflect that their name should reflect the LE/BE part and the unaligned part as well, I even think there are already macro somewhere for that. I would also add other formats like LE/BE 16/32/64 bits store/load all at once rather than each format when needed. If we add accessort, might as well add all the basic types. And finally those should really be inline functions in the .h, no point of doing a function call for that.
3) In include/osmocom/gsm/gsm_utils.h, for ms_a5n_support you take only one cm argument ... that means the app needs to know if it should give cm3 or cm2, I would take cm2 and cm3 separately and the app just has to give both. I might also extend ms_cm2_a5n_support and ms_cm3_a5n_support to return -1 in case it couldn't be determined (because the required test isn't in that classmark), but that last point is just a suggestion.
4) TAB vs SPACE indentation !
5) I would split the patch further between CM functions / buffere reversion / accessorts.
6) Is ROL16 really something we exepect to do at a lot of places ? just asking ... in anycase, also should probably be inline.
Cheers,
Sylvain
Thank you for review, comments are inline, rewrite in progress :)
07.04.2013 22:26, Sylvain Munaut пишет:
- Why do you need to add stddef.h ?
Required for size_t.
Also before of spacing change, you're removing an empty line that was there to separate the includes from the function comment.
- wrt to :
+uint16_t osmo_get2bytes(const uint8_t *a); +void osmo_64pack2pbit(uint64_t in, pbit_t *out);
Theses essentially look like integer accessors, one is a read for uint16_t in big endian and the other a store for uint64_t in little endian, but same basic idea. But you're using completely different naming schemes for both ... I think they should reflect that their name should reflect the LE/BE part and the unaligned part as well, I even think there are already macro somewhere for that. I would also add other formats like LE/BE 16/32/64 bits store/load all at once rather than each format when needed. If we add accessort, might as well add all the basic types. And finally those should really be inline functions in the .h, no point of doing a function call for that.
Is there some library I can use for that? It's indeed fairly trivial accessors but I didn't managed to find those in osmocom code.
- In include/osmocom/gsm/gsm_utils.h, for ms_a5n_support you take
only one cm argument ... that means the app needs to know if it should give cm3 or cm2, I would take cm2 and cm3 separately and the app just has to give both. I might also extend ms_cm2_a5n_support and ms_cm3_a5n_support to return -1 in case it couldn't be determined (because the required test isn't in that classmark), but that last point is just a suggestion.
Will fix, thanks.
- TAB vs SPACE indentation !
Doh! Could you remind me - what was the magic tool which takes care of indentation for kernel devs? Osmocom uses linux kernel code style, is it?
- I would split the patch further between CM functions / buffere
reversion / accessorts.
- Is ROL16 really something we exepect to do at a lot of places ?
just asking ... in anycase, also should probably be inline.
I thought it might but it turned out to be used only in single file. Should I hide it away?
- TAB vs SPACE indentation !
Doh! Could you remind me - what was the magic tool which takes care of indentation for kernel devs? Osmocom uses linux kernel code style, is it?
indent? But it has A LOT of options. Osmocom uses linux kernel coding style, right?
man indent [...]
The Linux style is used in the linux kernel code and drivers. Code generally has to follow the Linux coding style to be accepted. This style is equivalent to the following settings:
-nbad -bap -nbc -bbo -hnl -br -brs -c33 -cd33 -ncdb -ce -ci4 -cli0 -d0 -di1 -nfc1 -i8 -ip0 -l80 -lp -npcs -nprs -npsl -sai -saf -saw -ncs -nsc -sob -nfca -cp33 -ss -ts8 -il1 [...]
Does this command matches exactly osmocom coding style (can't try it myself now...)?
Hi Dario, Max and others,
On Mon, Apr 08, 2013 at 04:52:34PM +0200, Dario Lombardo wrote:
- TAB vs SPACE indentation !
Doh! Could you remind me - what was the magic tool which takes care of indentation for kernel devs? Osmocom uses linux kernel code style, is it?
indent? But it has A LOT of options. Osmocom uses linux kernel coding style, right?
"Lindent" which is part of every linux kenrel source code tree.
The easiest way is to always respect the coding style of the respective FOSS project while you make the modifications. Rationale: There is no guarantee that running the file through automatic tools 'lindent' will not create whitespace-changes outside your actual changes.
Regards, Harald
Hi Harald, and thanks for your answer. Strictly following coding style is the best way to keep code clean. But running indent/lindent against osmocom code reveals that in many parts it differs from "pure kernel" style. My question (maybe merely phylosofical) is: what should you do? Keep existing code, allowing those exception to lie there, hoping someone will clean them up manually, or run indent against the code, hoping it doesn't dirt the code?
On Wed, Apr 10, 2013 at 12:00 PM, Harald Welte laforge@gnumonks.org wrote:
The easiest way is to always respect the coding style of the respective FOSS project while you make the modifications. Rationale: There is no guarantee that running the file through automatic tools 'lindent' will not create whitespace-changes outside your actual changes.
I would not be worried about indent. Running it against a bunch of code could mess things up. But running on small pieces would be safe, under human control.
Just to clarify: I don't think you _need_ to run it. I'm just trying to understand an important point of view like yours.
Dario.
My question (maybe merely phylosofical) is: what should you do? Keep existing code, allowing those exception to lie there, hoping someone will clean them up manually, or run indent against the code, hoping it doesn't dirt the code?
In general we don't do pure formatting changes unless there is a vast improvement in readability.
If that part of the code is reworked, it'll get cleaned up.
Cheers,
Sylvain
Dear Max,
I'm getting back to this old thread about your kasumi, A5/3 and A5/4 patches.
There were lots of comments during the review in April, but I never saw a follow-up patchset that adressed those comments.
Particularly, from the discussion, I remember: 1) follow kernel coding style 2) whitespace / tab spacing 3) line wrapping 4) naming of helper routines which are generic accessor functions.
On Mon, Apr 08, 2013 at 04:10:32PM +0200, ☎ wrote:
+uint16_t osmo_get2bytes(const uint8_t *a); +void osmo_64pack2pbit(uint64_t in, pbit_t *out);
Theses essentially look like integer accessors, one is a read for uint16_t in big endian and the other a store for uint64_t in little endian, but same basic idea. But you're using completely different naming schemes for both ... I think they should reflect that their name should reflect the LE/BE part and the unaligned part as well, I even think there are already macro somewhere for that. I would also add other formats like LE/BE 16/32/64 bits store/load all at once rather than each format when needed. If we add accessort, might as well add all the basic types. And finally those should really be inline functions in the .h, no point of doing a function call for that.
Is there some library I can use for that? It's indeed fairly trivial accessors but I didn't managed to find those in osmocom code.
If there is no function / macro in libosmo* yet, then looking for kernel function names is always a good idea.
we have msgb_{put,get,pull}_u{8/16/32}() as accessors for working with message buffers. If msgb doesn't make sense in the context you are working in, then aligning the names to the same scheme might be another idea.
Regards, Harald
Hi Harald,
I actually got back to Max about this patch and I'll handle getting it merged and test it OTA as part of some security improvement stuff I'm working on.
Cheers,
Sylvain
On Fri, Nov 22, 2013 at 4:40 PM, Harald Welte laforge@gnumonks.org wrote:
Dear Max,
I'm getting back to this old thread about your kasumi, A5/3 and A5/4 patches.
There were lots of comments during the review in April, but I never saw a follow-up patchset that adressed those comments.
Particularly, from the discussion, I remember:
- follow kernel coding style
- whitespace / tab spacing
- line wrapping
- naming of helper routines which are generic accessor functions.
On Mon, Apr 08, 2013 at 04:10:32PM +0200, ☎ wrote:
+uint16_t osmo_get2bytes(const uint8_t *a); +void osmo_64pack2pbit(uint64_t in, pbit_t *out);
Theses essentially look like integer accessors, one is a read for uint16_t in big endian and the other a store for uint64_t in little endian, but same basic idea. But you're using completely different naming schemes for both ... I think they should reflect that their name should reflect the LE/BE part and the unaligned part as well, I even think there are already macro somewhere for that. I would also add other formats like LE/BE 16/32/64 bits store/load all at once rather than each format when needed. If we add accessort, might as well add all the basic types. And finally those should really be inline functions in the .h, no point of doing a function call for that.
Is there some library I can use for that? It's indeed fairly trivial accessors but I didn't managed to find those in osmocom code.
If there is no function / macro in libosmo* yet, then looking for kernel function names is always a good idea.
we have msgb_{put,get,pull}_u{8/16/32}() as accessors for working with message buffers. If msgb doesn't make sense in the context you are working in, then aligning the names to the same scheme might be another idea.
Regards, Harald --
- Harald Welte laforge@gnumonks.org http://laforge.gnumonks.org/
============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
Hi Sylvain,
On Fri, Nov 22, 2013 at 07:35:37PM +0100, Sylvain Munaut wrote:
I actually got back to Max about this patch and I'll handle getting it merged and test it OTA as part of some security improvement stuff I'm working on.
Ah, ok. thanks. Sorry in case I missed that on the list.
Hi Max,
I agree with Sylvain's comments, but like to add: The filename should not be gprs_gea.c but gprs_gea34.c or something along the lines. There are other ciphers (gea1/gea2) and that should somehow be reflected in the file name.
Regarding coding style: We generally open the curly braces on the same line as in "for (a;b;c) {". We also don't indent the "case" statements in a switch, e.g.:
switch (foo) { case A: foo = 1; break; }
I'm also not sure if the gea3/gea4 should become part of libosmogsm itself, or if they should simply exist in the form of a libosmo-crypt-gea34 or the like. Actually, I once created such a library (plugging into gprs_cipher_register()) using the reference implementation of the cipher. I'm not 100% sure on this, though. Does anyone have an opinion on this?
Even if it gea3/gea4 becomes part of libosmogsm, then I would like to have no direct functions exported to applications, but require applications to go through the gprs_cipher_* API. The same holds true for the A5/* family. Rather than having explicit function calls for each of the variants, I would love to have one set of functions with just a parameter or struct member defining the specific algorithm to be used by the implementation.
Regards, Harald
Thank you for review, comments are inline, rewrite in progress :)
08.04.2013 14:55, Harald Welte пишет:
I'm also not sure if the gea3/gea4 should become part of libosmogsm itself, or if they should simply exist in the form of a libosmo-crypt-gea34 or the like. Actually, I once created such a library (plugging into gprs_cipher_register()) using the reference implementation of the cipher. I'm not 100% sure on this, though. Does anyone have an opinion on this?
I think that they should because they are based on exactly the same kgcore primitive used by a5/3,4. Also I've tried to add register call (see gprs_gea.c) but when I call gprs_cipher_supported(GPRS_ALGO_GEA3) in gea_test.c it returns 0. How can I test that gea3 registered properly and is available via plugin interface?
Also, how should we change gprs_auth plugin api so it would work for 128 bit keys (GEA4)? Just change uint64_t to uint8_t or there's more to it?
Even if it gea3/gea4 becomes part of libosmogsm, then I would like to have no direct functions exported to applications, but require applications to go through the gprs_cipher_* API. The same holds true for the A5/* family. Rather than having explicit function calls for each of the variants, I would love to have one set of functions with just a parameter or struct member defining the specific algorithm to be used by the implementation.
I completely agree about hiding implementation of particular algorithms although I think that gea* belongs in libosmogsm - just like milenage and comp128 which re also supposed to be used via plugin api.
Is it enough to just remove function names from libosmogsm.map or there got to be "deeper" hiding?
Attached is the patch which address comments regarding ms_a5n_support() from previous patches. Also I'd like to check if code style is correct before addressing the rest of the comments.
Note that I've changed the type of "n" to unsigned - it's natural number anyway so there's no point using int in here.
Please review and merge if possible.
baseband-devel@lists.osmocom.org
-
Dario Lombardo -
Harald Welte -
Sylvain Munaut -
☎