diff mbox

openbsc[master]: sgsn: fix use of libosmocore GPRS encryption plugins from LL...

Message ID gerrit.1464617093261.I1f1b7454a0de5b7f4734aca4d03dbe67db5de189@gerrit.osmocom.org
State New
Headers show

Commit Message

gerrit-no-reply@lists.osmocom.org May 30, 2016, 2:04 p.m. UTC 
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

Comments

gerrit-no-reply@lists.osmocom.org May 30, 2016, 2:05 p.m. UTC | #1 
Patch Set 1:

I am not sure how to test this patch.
It is entirely untested.
gerrit-no-reply@lists.osmocom.org May 31, 2016, 9:38 a.m. UTC | #2 
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?
gerrit-no-reply@lists.osmocom.org May 31, 2016, noon UTC | #3 
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...
gerrit-no-reply@lists.osmocom.org June 1, 2016, 12:28 p.m. UTC | #4 
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
diff mbox

Patch

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);