intel / intel-ipsec-mb

Intel(R) Multi-Buffer Crypto for IPSec
BSD 3-Clause "New" or "Revised" License
288 stars 87 forks source link

potential uninitialized data, may be false positive find by static analysis #60

Closed ColinIanKing closed 3 years ago

ColinIanKing commented 4 years ago

Not sure if this is a false positive, but static analysis by Coverity picked up a potential use of uninitailized data. Report is as follows:

/lib/include/kasumi_internal.h

 849 static inline void
 850 kasumi_f8_1_buffer_bit(const kasumi_key_sched_t *pCtx, const uint64_t IV,
 851                       const void *pIn, void *pOut,
 852                       const uint32_t lengthInBits,
 853                       const uint32_t offsetInBits)
 854 {
 855 #ifdef SAFE_DATA
 856        CLEAR_SCRATCH_SIMD_REGS();
 857 #endif /* SAFE_DATA */
 858
 859        const uint8_t *pBufferIn = (const uint8_t *) pIn;
 860        uint8_t *pBufferOut = (uint8_t *) pOut;
 861        uint32_t cipherLengthInBits = lengthInBits;
 862        uint32_t blkcnt;
 863        uint64_t shiftrem = 0;
 864        kasumi_union_t a, b, c; /* the modifier */
 865        const uint8_t *pcBufferIn = pBufferIn + (offsetInBits / 8);
 866        uint8_t *pcBufferOut = pBufferOut + (offsetInBits / 8);
 867        /* Offset into the first byte (0 - 7 bits) */
 868        uint32_t remainOffset = offsetInBits % 8;
 869        uint32_t byteLength = (cipherLengthInBits + 7) / 8;
 870        SafeBuf safeOutBuf = {0};

    1. var_decl: Declaring variable safeInBuf without initializer.
 871        SafeBuf safeInBuf;
 872
 873        /* IV Endianity  */
 874        a.b64[0] = BSWAP64(IV);
 875
 876        /* First encryption to create modifier */
 877        kasumi_1_block(pCtx->msk16, a.b16);
 878
 879        /* Final initialisation steps */
 880        blkcnt = 0;
 881        b.b64[0] = a.b64[0];
 882        /* Now run the block cipher */
 883
 884        /* Start with potential partial block (due to offset and length) */
 885        kasumi_1_block(pCtx->sk16, b.b16);
 886        c.b64[0] = b.b64[0] >> remainOffset;
 887        /* Only one block to encrypt */

    2. Condition cipherLengthInBits < 64 - remainOffset, taking true branch.
 888        if (cipherLengthInBits < (64 - remainOffset)) {
 889                byteLength = (cipherLengthInBits + 7) / 8;
 890                memcpy_keystrm(safeInBuf.b8, pcBufferIn, byteLength);
 891                /*
 892                 * If operation is Out-of-place and there is offset
 893                 * to be applied, "remainOffset" bits from the output buffer
 894                 * need to be preserved (only applicable to first byte,
 895                 * since remainOffset is up to 7 bits)
 896                 */

    3. Condition pIn != pOut, taking true branch.
    4. Condition remainOffset, taking true branch.
 897                if ((pIn != pOut) && remainOffset) {
 898                        const uint8_t mask8 =
 899                                (const uint8_t)(1 << (8 - remainOffset)) - 1;
 900

 Uninitialized scalar variable (UNINIT)
    5. uninit_use: Using uninitialized value safeInBuf.b8[0].

 901                        safeInBuf.b8[0] = (safeInBuf.b8[0] & mask8) |
 902                                        (pcBufferOut[0] & ~mask8);
 903                }
 904
pablodelara commented 4 years ago

Looks like a false positive, safeInBuf will get always valid bytes from pcBufferIn, so it doesn't matter if it is not initialized.

pablodelara commented 3 years ago

I'll close this, let me know if you disagree about it and we can reopen it.

ColinIanKing commented 3 years ago

Sure, looks fine to me. Sorry about the false positive report wasting effort.

pablodelara commented 3 years ago

No need to apologize! Thank you for reporting all this stuff, it really helps us!