Review at https://gerrit.osmocom.org/128
sgsn: fix use of libosmocore GPRS encryption plugins from LLC layer
Instead of passing the uint64_t kc bytes wrongly interpreted as memory address, pass its actual kc bytes by casting via (uint8_t*)&kc.
Change-Id: I1f1b7454a0de5b7f4734aca4d03dbe67db5de189 --- M openbsc/src/gprs/gprs_llc.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/28/128/1
diff --git a/openbsc/src/gprs/gprs_llc.c b/openbsc/src/gprs/gprs_llc.c index 4cf5163..e3c0726 100644 --- a/openbsc/src/gprs/gprs_llc.c +++ b/openbsc/src/gprs/gprs_llc.c @@ -417,7 +417,7 @@
/* Compute the keystream that we need to XOR with the data */ rc = gprs_cipher_run(cipher_out, crypt_len, lle->llme->algo, - kc, iv, GPRS_CIPH_SGSN2MS); + (uint8_t*)&kc, iv, GPRS_CIPH_SGSN2MS); if (rc < 0) { LOGP(DLLC, LOGL_ERROR, "Error crypting UI frame: %d\n", rc); msgb_free(msg); @@ -623,7 +623,7 @@ iv = gprs_cipher_gen_input_ui(iov_ui, lle->sapi, llhp.seq_tx, lle->oc_ui_recv); rc = gprs_cipher_run(cipher_out, crypt_len, lle->llme->algo, - kc, iv, GPRS_CIPH_MS2SGSN); + (uint8_t*)&kc, iv, GPRS_CIPH_MS2SGSN); if (rc < 0) { LOGP(DLLC, LOGL_ERROR, "Error decrypting frame: %d\n", rc);
Patch Set 1:
I am not sure how to test this patch. It is entirely untested.
Patch Set 1:
(1 comment)
https://gerrit.osmocom.org/#/c/128/1/openbsc/src/gprs/gprs_llc.c File openbsc/src/gprs/gprs_llc.c:
Line 413: uint64_t kc = *(uint64_t *)&lle->llme->kc; If I look at:
unsigned gprs_cipher_key_length(enum gprs_ciph_algo algo) of libosmocore
Does key_length relate to sizeof(kc) here? If yes then uint64_t is even too small besides that you are right and we should pass the address.. but maybe just use &lle->llme->kc directly?
Patch Set 1:
It looked to me like the code wants to make sure not to change the llme->kc by passing it into the function. So it wants to copy to a local buffer and pass *its* address instead.
If we'd want to keep it that way then, yes, I see now and agree about the size, and we should use a malloc(gprs_cipher_key_length(..))
...or just pass &kc directly.
I'm not sure how to decide, just caught this by accident and am not familiar with this code...
Patch Set 1: Code-Review-1
Instead of casting uint8_t *kc to uint64_t and back we should use it directly. Also the size of kc in gprs-llc_llme should be adjusted to accommodate for GEA4. Related ticket: OS#1582