[v3] drivers: fix to replace strcat with strlcat
Checks
Commit Message
Strcat does not check the destination length and there might be
chances of string overflow so instead of strcat, strlcat is used.
Fixes: 540a211084 ("bnx2x: driver core")
Fixes: e163c18a15 ("net/i40e: update ptype and pctype info")
Fixes: ef28aa96e5 ("net/nfp: support multiprocess")
Fixes: 6f4eec2565 ("test/crypto: enhance scheduler unit tests")
Cc: stable@dpdk.org
Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
---
v3: Instead of strncat, used strlcat.
v2: Instead of strncat, used snprintf.
---
drivers/net/bnx2x/bnx2x.c | 6 ++++--
drivers/net/i40e/i40e_ethdev.c | 6 ++++--
drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 10 ++++++----
test/test/test_cryptodev.c | 5 ++++-
4 files changed, 18 insertions(+), 9 deletions(-)
Comments
On 2/27/2019 6:02 AM, Chaitanya Babu Talluri wrote:
> Strcat does not check the destination length and there might be
> chances of string overflow so instead of strcat, strlcat is used.
>
> Fixes: 540a211084 ("bnx2x: driver core")
> Fixes: e163c18a15 ("net/i40e: update ptype and pctype info")
> Fixes: ef28aa96e5 ("net/nfp: support multiprocess")
> Fixes: 6f4eec2565 ("test/crypto: enhance scheduler unit tests")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
> ---
> v3: Instead of strncat, used strlcat.
> v2: Instead of strncat, used snprintf.
> ---
> drivers/net/bnx2x/bnx2x.c | 6 ++++--
> drivers/net/i40e/i40e_ethdev.c | 6 ++++--
> drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 10 ++++++----
> test/test/test_cryptodev.c | 5 ++++-
> 4 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c
> index 4c775c163..e418fd7d1 100644
> --- a/drivers/net/bnx2x/bnx2x.c
> +++ b/drivers/net/bnx2x/bnx2x.c
> @@ -11734,13 +11734,15 @@ static const char *get_bnx2x_flags(uint32_t flags)
>
> for (i = 0; i < 5; i++)
> if (flags & (1 << i)) {
> - strcat(flag_str, flag[i]);
> + strlcat(flag_str, flag[i],
> + BNX2X_INFO_STR_MAX - strlen(flag_str) - 1);
Hi Chaitanya,
I am not sure if this is correct usage of `strlcat`, can you please check its
man page [1], my concern is specially following part:
"... Unlike those functions, strlcpy() and strlcat() take the full size of the
buffer (not just the length) and ... "
[1]
https://linux.die.net/man/3/strlcat
On Wed, Feb 27, 2019 at 06:02:51AM +0000, Chaitanya Babu Talluri wrote:
> Strcat does not check the destination length and there might be
> chances of string overflow so instead of strcat, strlcat is used.
>
> Fixes: 540a211084 ("bnx2x: driver core")
> Fixes: e163c18a15 ("net/i40e: update ptype and pctype info")
> Fixes: ef28aa96e5 ("net/nfp: support multiprocess")
> Fixes: 6f4eec2565 ("test/crypto: enhance scheduler unit tests")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
> ---
> v3: Instead of strncat, used strlcat.
> v2: Instead of strncat, used snprintf.
> ---
> drivers/net/bnx2x/bnx2x.c | 6 ++++--
> drivers/net/i40e/i40e_ethdev.c | 6 ++++--
> drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 10 ++++++----
> test/test/test_cryptodev.c | 5 ++++-
> 4 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c
> index 4c775c163..e418fd7d1 100644
> --- a/drivers/net/bnx2x/bnx2x.c
> +++ b/drivers/net/bnx2x/bnx2x.c
> @@ -11734,13 +11734,15 @@ static const char *get_bnx2x_flags(uint32_t flags)
>
> for (i = 0; i < 5; i++)
> if (flags & (1 << i)) {
> - strcat(flag_str, flag[i]);
> + strlcat(flag_str, flag[i],
> + BNX2X_INFO_STR_MAX - strlen(flag_str) - 1);
> flags ^= (1 << i);
> }
> if (flags) {
> static char unknown[BNX2X_INFO_STR_MAX];
> snprintf(unknown, 32, "Unknown flag mask %x", flags);
> - strcat(flag_str, unknown);
> + strlcat(flag_str, unknown,
> + BNX2X_INFO_STR_MAX - strlen(flag_str) - 1);
> }
This doesn't look right to me. "Strlcat" takes the saner approach of having
the length parameter being total length so subtraction etc. should not be
necessary. I think this should just be
"strlcat(flag_str, unknown, BNX2X_INFO_STR_MAX);"
> -----Original Message-----
> From: Chaitanya Babu, TalluriX
> Sent: Wednesday, February 27, 2019 6:03 AM
> To: dev@dpdk.org
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index dca61f03a..fac4e943f 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -12201,8 +12201,10 @@ i40e_update_customized_pctype(struct
> rte_eth_dev *dev, uint8_t *pkg,
> for (n = 0; n < proto_num; n++) {
> if (proto[n].proto_id != proto_id)
> continue;
> - strcat(name, proto[n].name);
> - strcat(name, "_");
> + strlcat(name, proto[n].name,
> + sizeof(name) - strlen(name) - 1);
> + strlcat(name, "_",
> + sizeof(name) - strlen(name) - 1);
> break;
> }
> }
You need to include rte_string_fns.h here , check the build failure at below link.
http://patches.dpdk.org/patch/50535/
In other files the header is included indirectly, but I suggest include this explicitly to avoid any header dependencies .
Also, commit message heading should be "fix possible overflow with strlcat"?
Thanks,
Reshma
@@ -11734,13 +11734,15 @@ static const char *get_bnx2x_flags(uint32_t flags)
for (i = 0; i < 5; i++)
if (flags & (1 << i)) {
- strcat(flag_str, flag[i]);
+ strlcat(flag_str, flag[i],
+ BNX2X_INFO_STR_MAX - strlen(flag_str) - 1);
flags ^= (1 << i);
}
if (flags) {
static char unknown[BNX2X_INFO_STR_MAX];
snprintf(unknown, 32, "Unknown flag mask %x", flags);
- strcat(flag_str, unknown);
+ strlcat(flag_str, unknown,
+ BNX2X_INFO_STR_MAX - strlen(flag_str) - 1);
}
return flag_str;
}
@@ -12201,8 +12201,10 @@ i40e_update_customized_pctype(struct rte_eth_dev *dev, uint8_t *pkg,
for (n = 0; n < proto_num; n++) {
if (proto[n].proto_id != proto_id)
continue;
- strcat(name, proto[n].name);
- strcat(name, "_");
+ strlcat(name, proto[n].name,
+ sizeof(name) - strlen(name) - 1);
+ strlcat(name, "_",
+ sizeof(name) - strlen(name) - 1);
break;
}
}
@@ -73,6 +73,8 @@
#define NFP_PCIE_CPP_BAR_PCIETOCPPEXPBAR(bar, slot) \
(((bar) * 8 + (slot)) * 4)
+#define LOCKFILE_HOME_PATH 256
+
/*
* Define to enable a bit more verbose debug output.
* Set to 1 to enable a bit more verbose debug output.
@@ -685,11 +687,11 @@ nfp_acquire_secondary_process_lock(struct nfp_pcie_user *desc)
* driver is used because that implies root user.
*/
home_path = getenv("HOME");
- lockfile = calloc(strlen(home_path) + strlen(lockname) + 1,
- sizeof(char));
+ lockfile = calloc(LOCKFILE_HOME_PATH + strlen(lockname) + 1,
+ sizeof(char));
- strcat(lockfile, home_path);
- strcat(lockfile, "/.lock_nfp_secondary");
+ snprintf(lockfile, LOCKFILE_HOME_PATH + strlen(lockname),
+ "%s%s", home_path, lockname);
desc->secondary_lock = open(lockfile, O_RDWR | O_CREAT | O_NONBLOCK,
0666);
if (desc->secondary_lock < 0) {
@@ -11,6 +11,8 @@
#include <rte_memcpy.h>
#include <rte_pause.h>
#include <rte_bus_vdev.h>
+#include <string.h>
+#include <rte_string_fns.h>
#include <rte_crypto.h>
#include <rte_cryptodev.h>
@@ -375,7 +377,8 @@ testsuite_setup(void)
snprintf(vdev_args, sizeof(vdev_args),
"%s%d", temp_str, i);
strcpy(temp_str, vdev_args);
- strcat(temp_str, ";");
+ strlcat(temp_str, ";",
+ VDEV_ARGS_SIZE - strlen(temp_str) - 1);
slave_core_count++;
socket_id = lcore_config[i].socket_id;
}