Closed jas4711 closed 11 months ago
I have just run this on Ubuntu x86_64 without valgrind. But I tested the library and gpshell with valgrind. Could cmocka be the issue?
I have found 2 incorrect header declaratiosn of the byte array size. Please check again. Also the fixed bate ararys which I'm usign are causing a lot of of warning wlthough I don't understand why it is complaining since the size is OK:
/home/foobar/projects/globalplatform/globalplatform/src/globalplatform.c:4987:26: note: referencing argument 4 of type ‘BYTE ’ {aka ‘unsigned char ’} In file included from /home/foobar/projects/globalplatform/globalplatform/src/globalplatform.c:56: /home/foobar/projects/globalplatform/globalplatform/src/crypto.h:90:19: note: in a call to function ‘create_session_key_SCP02’ 90 | OPGP_ERROR_STATUS create_session_key_SCP02(BYTE key[16], BYTE constant[2], | ^
~~~~~~~ /home/foobar/projects/globalplatform/globalplatform/src/globalplatform.c:4993:26: warning: ‘create_session_key_SCP02’ accessing 16 bytes in a region of size 2 [-Wstringop-overflow=] 4993 | status = create_session_key_SCP02(DEK, DEKDerivationConstant, sequenceCounter, secInfo->dataEncryptionSessionKey); | ^~~~~~~~~~~~~~~~~~~~~~~~
I think using foo[17]
or foo[42]
in a C function parameter prototype is equivalent: they are both converted to *foo and the size value is lost. It is possible to sprinkle a 'static' in there, but this is rarely done so I wouldn't recommend it, and it disallows passing NULL, see for example: https://hamberg.no/erlend/posts/2013-02-18-static-array-indices.html
I didn't analyze the first warning, but maybe it goes away if you convert the C function prototype to foo[]
or even *foo
? The second warning seems like maybe the caller used a too short buffer for the 2-byte constant array?
So I think your patch is essentially a no-op and doesn't change anything. If the buffer size has to be changed, it has to be changed in the caller to the function. I re-ran the valgrind command above, and the output is still the same.
I have tried to analyse scp01. I assum the "still reachable leaks" are not a problem and the problem is coming from "Uninitialised value was created by a stack allocation"? Not sure if the Valgrind traces are all correct, but I have not found any explanantion what could be wrong with these lines. Is the amd64 error related to the errors on other platforms?
Yes the "Conditional jump or move depends on uninitialised value" normally indicates real source-code level problems.
It is not certain that fixing the amd64 valgrind error will fix the problem on the other platform, but in my experience valgrind is a good tool to find things that merely accidentally work on one platform but actually breaks practically on another platform. So I'd give it a 75% probability that fixing the valgrind problem will fix the platform problems too. And usually debugging and fixing valgrind errors on amd64 is easier than debugging and fixing native problem on other hardware, although maybe this changed in the last few years... the Compile Farm project https://portal.cfarm.net/machines/list/ has many machines available for development if you find it easier to debug it natively.
I have fixed the "Uninitialised value was created by a stack allocation" issues. Please give it a try.
Thank you! I can confirm valgrind errors are gone now. I'm preparing a new upload to Debian to test on other architectures, although there seems to be some problem with the "pandoc" package in Debian causing build problems.
Great. Not sure, where I use pandoc, I think it was the man page. If this is mandatory to have in Debian, then a solution must be found, otherwise maybe also the produced output can be included directly without runnig pandoc.
It builds with self-checks on many platforms now! So I think your patch fixed it.
https://buildd.debian.org/status/package.php?p=globalplatform
However s390x fails, but it is different:
4/5 Test #4: cryptoTest .......................***Failed 0.00 sec
[==========] Running 3 test(s).
[ RUN ] test_aes_cmac_128
[ OK ] test_aes_cmac_128
[ RUN ] test_aes_cmac_192
[ OK ] test_aes_cmac_192
[ RUN ] test_read_public_rsa_key
[ ERROR ] --- 0x100010000000000 != 0x10001
[ LINE ] --- ./globalplatform/src/cryptoTest.c:99: error: Failure!
[ FAILED ] test_read_public_rsa_key
[==========] 3 test(s) run.
[ PASSED ] 2 test(s).
[ FAILED ] 1 test(s), listed below:
[ FAILED ] test_read_public_rsa_key
Any ideas? See complete log here:
It seems like stdout/stderr ordering seems a bit garbled due to buffering.
The prefix is identical:
0x100010000000000 != 0x10001
This looks like 65535, the common RSA exponent. I assume it is an alignment issue or data type issue then.
It is this line:
BN_bn2bin(e, ((unsigned char *)rsaExponent));
So, a little endina big endian problem, it seems.
The patch in #88 resolves the problem, and now it builds on all platforms in Debian with self-checks, see logs:
https://buildd.debian.org/status/package.php?p=globalplatform https://buildd.debian.org/status/fetch.php?pkg=globalplatform&arch=s390x&ver=2.3.1%2Bdfsg-5&stamp=1702631621&raw=0
Hi. The globalplatform package just entered Debian, and it is attempted to be built on several architectures, see:
https://buildd.debian.org/status/package.php?p=globalplatform
But the scp01Test and scp02Test self-checks fails with output like this on several architectures:
I'm able to reproduce the problem under valgrind on amd64 too. (To get valgrind to work, I had to disable -fsanitize=address from DEBUG build.)
The C code triggers plenty of type and bound warnings here, and I suspect there is some 'unsigned' vs 'signed' mismatch somewhere causing errors. Before spending too much time debugging, I thought I'd open an issue about it if you can reproduce the problem and debug it more easily than I can.
/Simon