intel / idxd-config

Accel-config / libaccel-config
Other
56 stars 35 forks source link

accel-config: Fix issues reported by coverity scan #67

Open snits opened 1 month ago

snits commented 1 month ago

It is possible for iaa_do_crypto to return an error value so the returned value should be checked when it is called. memcmp expects a size_t for the number of bytes to compare, which is an unsigned integer. If an error value is returned by iaa_do_crypto, this results in calling memcmp with a rather large size as the negative value is converted to a size_t. This was detected by OpenScanHub service that runs when an official build of a package is submitted.

Signed-off-by: Jerry Snitselaar jsnitsel@redhat.com

jmiao2018 commented 1 month ago

In theory, it seems possible. Can you provide specific log testing log information ? Thank you

snits commented 1 month ago

I don't have a log of it happening. It was covscan running and statically analyzing the code, but just a quick look at iaa_do_crypto shows 3 places where it can return -1, which will then get passed to memcmp.

snits commented 1 month ago

Added a 2nd commit to clean up a resource leak in accfg_wq_get_occupancy where it wasn't closing a file prior to returning. Another coverity scan report from the start of RHEL10 work.

snits commented 1 month ago

In theory, it seems possible. Can you provide specific log testing log information ? Thank you

If you are wanting what coverity scan reported it is the following:




Error: OVERRUN (CWE-119):
idxd-config-accel-config-v4.1.6/test/iaa.c:2624:2: return_constant: Function call "iaa_do_crypto(tsk->output, tsk->src1, tsk->xfer_size, (uint8_t *)iaa_crypto_aecs->aes_key_low, (uint8_t *)iaa_crypto_aecs->counter_iv, key_size, iaa_crypto_aecs->crypto_algorithm, 1)" may return -1.
idxd-config-accel-config-v4.1.6/test/iaa.c:2624:2: assignment: Assigning: "expected_len" = "iaa_do_crypto(tsk->output, tsk->src1, tsk->xfer_size, (uint8_t *)iaa_crypto_aecs->aes_key_low, (uint8_t *)iaa_crypto_aecs->counter_iv, key_size, iaa_crypto_aecs->crypto_algorithm, 1)". The value of "expected_len" is now -1.
idxd-config-accel-config-v4.1.6/test/iaa.c:2628:2: overrun-buffer-arg: Calling "memcmp" with "tsk->dst1" and "expected_len" is suspicious because of the very large index, 18446744073709551615. The index may be due to a negative parameter being interpreted as unsigned.
# 2626|                      (uint8_t *)iaa_crypto_aecs->counter_iv,
# 2627|                      key_size, iaa_crypto_aecs->crypto_algorithm, 1);
# 2628|->   rc = memcmp(tsk->dst1, tsk->output, expected_len);
# 2629|   
# 2630|     if (!mismatch_expected) {

Error: OVERRUN (CWE-119):
idxd-config-accel-config-v4.1.6/test/iaa.c:2624:2: return_constant: Function call "iaa_do_crypto(tsk->output, tsk->src1, tsk->xfer_size, (uint8_t *)iaa_crypto_aecs->aes_key_low, (uint8_t *)iaa_crypto_aecs->counter_iv, key_size, iaa_crypto_aecs->crypto_algorithm, 1)" may return -1.
idxd-config-accel-config-v4.1.6/test/iaa.c:2624:2: assignment: Assigning: "expected_len" = "iaa_do_crypto(tsk->output, tsk->src1, tsk->xfer_size, (uint8_t *)iaa_crypto_aecs->aes_key_low, (uint8_t *)iaa_crypto_aecs->counter_iv, key_size, iaa_crypto_aecs->crypto_algorithm, 1)". The value of "expected_len" is now -1.
idxd-config-accel-config-v4.1.6/test/iaa.c:2628:2: overrun-buffer-arg: Calling "memcmp" with "tsk->output" and "expected_len" is suspicious because of the very large index, 18446744073709551615. The index may be due to a negative parameter being interpreted as unsigned.
# 2626|                      (uint8_t *)iaa_crypto_aecs->counter_iv,
# 2627|                      key_size, iaa_crypto_aecs->crypto_algorithm, 1);
# 2628|->   rc = memcmp(tsk->dst1, tsk->output, expected_len);
# 2629|   
# 2630|     if (!mismatch_expected) {

Error: OVERRUN (CWE-119):
idxd-config-accel-config-v4.1.6/test/iaa.c:2672:2: return_constant: Function call "iaa_do_crypto(tsk->output, tsk->src1, tsk->xfer_size, (uint8_t *)iaa_crypto_aecs->aes_key_low, (uint8_t *)iaa_crypto_aecs->counter_iv, key_size, iaa_crypto_aecs->crypto_algorithm, 0)" may return -1.
idxd-config-accel-config-v4.1.6/test/iaa.c:2672:2: assignment: Assigning: "expected_len" = "iaa_do_crypto(tsk->output, tsk->src1, tsk->xfer_size, (uint8_t *)iaa_crypto_aecs->aes_key_low, (uint8_t *)iaa_crypto_aecs->counter_iv, key_size, iaa_crypto_aecs->crypto_algorithm, 0)". The value of "expected_len" is now -1.
idxd-config-accel-config-v4.1.6/test/iaa.c:2676:2: overrun-buffer-arg: Calling "memcmp" with "tsk->dst1" and "expected_len" is suspicious because of the very large index, 18446744073709551615. The index may be due to a negative parameter being interpreted as unsigned.
# 2674|                      (uint8_t *)iaa_crypto_aecs->counter_iv,
# 2675|                      key_size, iaa_crypto_aecs->crypto_algorithm, 0);
# 2676|->   rc = memcmp(tsk->dst1, tsk->output, expected_len);
# 2677|   
# 2678|     if (!mismatch_expected) {

Error: OVERRUN (CWE-119):
idxd-config-accel-config-v4.1.6/test/iaa.c:2672:2: return_constant: Function call "iaa_do_crypto(tsk->output, tsk->src1, tsk->xfer_size, (uint8_t *)iaa_crypto_aecs->aes_key_low, (uint8_t *)iaa_crypto_aecs->counter_iv, key_size, iaa_crypto_aecs->crypto_algorithm, 0)" may return -1.
idxd-config-accel-config-v4.1.6/test/iaa.c:2672:2: assignment: Assigning: "expected_len" = "iaa_do_crypto(tsk->output, tsk->src1, tsk->xfer_size, (uint8_t *)iaa_crypto_aecs->aes_key_low, (uint8_t *)iaa_crypto_aecs->counter_iv, key_size, iaa_crypto_aecs->crypto_algorithm, 0)". The value of "expected_len" is now -1.
idxd-config-accel-config-v4.1.6/test/iaa.c:2676:2: overrun-buffer-arg: Calling "memcmp" with "tsk->output" and "expected_len" is suspicious because of the very large index, 18446744073709551615. The index may be due to a negative parameter being interpreted as unsigned.
# 2674|                      (uint8_t *)iaa_crypto_aecs->counter_iv,
# 2675|                      key_size, iaa_crypto_aecs->crypto_algorithm, 0);
# 2676|->   rc = memcmp(tsk->dst1, tsk->output, expected_len);
# 2677|   
# 2678|     if (!mismatch_expected) {

For the resource leak it reported:


Error: RESOURCE_LEAK (CWE-772):
idxd-config-accel-config-v4.1.6/accfg/lib/libaccfg.c:1925:2: open_fn: Returning handle opened by "open". [Note: The source code implementation of the function has been overridden by a user model.]
idxd-config-accel-config-v4.1.6/accfg/lib/libaccfg.c:1925:2: var_assign: Assigning: "dfd" = handle returned from "open(wq->wq_path, 2097152)".
idxd-config-accel-config-v4.1.6/accfg/lib/libaccfg.c:1929:2: noescape: Resource "dfd" is not freed or pointed-to in "accfg_get_param_long".
idxd-config-accel-config-v4.1.6/accfg/lib/libaccfg.c:1929:2: leaked_handle: Handle variable "dfd" going out of scope leaks the handle.
# 1927|         return -ENXIO;
# 1928|   
# 1929|->   return accfg_get_param_long(ctx, dfd, "occupancy");
# 1930|   }
# 1931|