search

intel / intel-ipsec-mb

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

out-of-bounds read detected with static analysis on tmp.byte[] #36

Closed ColinIanKing closed 5 years ago

ColinIanKing commented 5 years ago

Static analysis on function shift_rows() in source no-aesni/aesni_emu.c has found a potential out-of-bounds read on an array. The full coverity scan static analysis report is below:

251 static void shift_rows(union xmm_reg dst, const union xmm_reg src) 252 { 253 / cyclic shift last 3 rows of the input / 254 int j; 255 union xmm_reg tmp = src; 256 257 / bytes to matrix: 258 0 1 2 3 < columns (i) 259 ----------+ 260 0 4 8 C | 0 < rows (j) 261 1 5 9 D | 1 262 2 6 A E | 2 263 3 7 B F | 3 264 265 THIS IS THE KEY: progressively move elements to HIGHER 266 numbered columnar values within a row. 267 268 Each dword is a column with the MSB as the bottom element 269 i is the column index, selects the dword 270 j is the row index, 271 we shift row zero by zero, row 1 by 1 and row 2 by 2 and 272 row 3 by 3, cyclically */

  1. Condition j < 4, taking true branch.
  2. Condition j < 4, taking true branch.
  3. cond_at_most: Checking j < 4 implies that j may be up to 3 on the true branch.

273 for (j = 0; j < MAX_DWORDS_PER_XMM; j++) { 274 int i; 275

  1. Condition i < 4, taking true branch.
  2. Condition i < 4, taking true branch.
  3. Condition i < 4, taking false branch.
  4. Condition i < 4, taking true branch.
  5. Condition i < 4, taking true branch.
  6. cond_at_most: Checking i < 4 implies that i may be up to 3 on the true branch.

276 for (i = 0; i < MAX_DWORDS_PER_XMM; i++)

  1. Jumping back to the beginning of the loop.
  2. Jumping back to the beginning of the loop.
  3. Jumping back to the beginning of the loop.
  4. identity_transfer: Passing i + j as argument 1 to function wrap_pos, which returns that argument. CID 84512 (#1 of 1): Out-of-bounds read (OVERRUN)15. overrun-local: Overrunning array tmp.byte of 16 bytes at byte offset 27 using index wrap_pos(i + j) * 4U + j (which evaluates to 27).

277 dst->byte[i4+j] = tmp.byte[wrap_pos(i+j)4+j];

  1. Jumping back to the beginning of the loop. 278 } 279 280 }
mdcornu commented 5 years ago

Hi Colin,

Thanks for highlighting.

This particular issue has been detected in the past and I believe this is a false positive. Since both i and j will be in the range 0-3, the wrap_pos() function will ensure wrap_pos(i+j)*4+j will always evaluate to a value between 0 and 15.

Regards, Marcel

mdcornu commented 5 years ago

Closing this issue. Feel free to reopen again if necessary.

Regards, Marcel