7 Apr
2013
7 Apr
'13
3:08 p.m.
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.
--
best regards,
Max, http://fairwaves.ru
7 Apr
7 Apr
10:26 p.m.
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
8 Apr
8 Apr
4:10 p.m.
Thank you for review, comments are inline, rewrite in progress :)
07.04.2013 22:26, Sylvain Munaut пишет:
1) 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.
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.
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.
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.
Will fix, thanks.
4) 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?
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.
I thought it might but it turned out to be used only in single file. Should I hide it
away?
--
best regards,
Max, http://fairwaves.ru
4:52 p.m.
4) 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...)?
10 Apr
10 Apr
noon
Hi Dario, Max and others,
On Mon, Apr 08, 2013 at 04:52:34PM +0200, Dario Lombardo wrote:
"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
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
4) 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?
10:56 p.m.
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(a)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.
11 Apr
11 Apr
12:42 a.m.
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
22 Nov
22 Nov
4:40 p.m.
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:
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(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
+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.
7:35 p.m.
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(a)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:
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:
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(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
+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.
8:50 p.m.
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.
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
8 Apr
8 Apr
2:55 p.m.
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
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
4:20 p.m.
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?
--
best regards,
Max, http://fairwaves.ru
9 Apr
9 Apr
5:23 p.m.
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.
--
best regards,
Max, http://fairwaves.ru
4042
days inactive
4044
days old
baseband-devel@lists.osmocom.org
12 comments
4 participants
participants (4)
-
Dario Lombardo -
Harald Welte -
Sylvain Munaut -
☎