drivers: fix to replace strcat with strncat
Checks
Commit Message
Strcat does not check the destination length and there might be
chances of string overflow so insted of strcat, strncat 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>
---
drivers/net/bnx2x/bnx2x.c | 6 ++++--
drivers/net/i40e/i40e_ethdev.c | 6 ++++--
drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 8 +++++---
test/test/test_cryptodev.c | 3 ++-
4 files changed, 15 insertions(+), 8 deletions(-)
Comments
On 1/14/2019 6:04 AM, Chaitanya Babu Talluri wrote:
> Strcat does not check the destination length and there might be
> chances of string overflow so insted of strcat, strncat 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>
<...>
> @@ -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,
> + lockfile = calloc(LOCKFILE_HOME_PATH + strlen(lockname) + 1,
> sizeof(char));
>
> - strcat(lockfile, home_path);
> - strcat(lockfile, "/.lock_nfp_secondary");
> + strncat(lockfile, home_path, LOCKFILE_HOME_PATH);
> + strncat(lockfile, lockname, strlen(lockfile));
I guess this need to be 'LOCKFILE_HOME_PATH - strlen(lockfile) - 1' instead.
But also this can be implemented as 'snprintf()'
Since 'lockfile' allocated dynamically based on sizes of existing strings, using
'lockname' instead of "/.lock_nfp_secondary" will show that there won't be any
overflow but tools still may be complaining about 'strcat' usage.
On Mon, Jan 14, 2019 at 06:04:35AM +0000, Chaitanya Babu Talluri wrote:
> Strcat does not check the destination length and there might be
> chances of string overflow so insted of strcat, strncat 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>
> ---
> drivers/net/bnx2x/bnx2x.c | 6 ++++--
> drivers/net/i40e/i40e_ethdev.c | 6 ++++--
> drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 8 +++++---
> test/test/test_cryptodev.c | 3 ++-
> 4 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c
> index 4c775c163..0e1e6447a 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]);
> + strncat(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);
> + strncat(flag_str, unknown,
> + BNX2X_INFO_STR_MAX - strlen(flag_str) - 1);
> }
> return flag_str;
> }
While I believe this is correct, this (and the changes below) would be a
lot neater using strlcat function. We should perhaps look to add strlcat
alongside strlcpy for DPDK use.
/Bruce
On Mon, 14 Jan 2019 13:29:38 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 1/14/2019 6:04 AM, Chaitanya Babu Talluri wrote:
> > Strcat does not check the destination length and there might be
> > chances of string overflow so insted of strcat, strncat 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>
>
> <...>
>
> > @@ -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,
> > + lockfile = calloc(LOCKFILE_HOME_PATH + strlen(lockname) + 1,
> > sizeof(char));
> >
> > - strcat(lockfile, home_path);
> > - strcat(lockfile, "/.lock_nfp_secondary");
> > + strncat(lockfile, home_path, LOCKFILE_HOME_PATH);
> > + strncat(lockfile, lockname, strlen(lockfile));
>
> I guess this need to be 'LOCKFILE_HOME_PATH - strlen(lockfile) - 1' instead.
> But also this can be implemented as 'snprintf()'
>
> Since 'lockfile' allocated dynamically based on sizes of existing strings, using
> 'lockname' instead of "/.lock_nfp_secondary" will show that there won't be any
> overflow but tools still may be complaining about 'strcat' usage.
>
>
Why not use vasprintf() instead of doing manual construction?
14/01/2019 15:21, Bruce Richardson:
> On Mon, Jan 14, 2019 at 06:04:35AM +0000, Chaitanya Babu Talluri wrote:
> > Strcat does not check the destination length and there might be
> > chances of string overflow so insted of strcat, strncat is used.
[...]
>
> While I believe this is correct, this (and the changes below) would be a
> lot neater using strlcat function. We should perhaps look to add strlcat
> alongside strlcpy for DPDK use.
Yes it looks reasonnable.
14/01/2019 17:24, Stephen Hemminger:
> On Mon, 14 Jan 2019 13:29:38 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> > On 1/14/2019 6:04 AM, Chaitanya Babu Talluri wrote:
> > > Strcat does not check the destination length and there might be
> > > chances of string overflow so insted of strcat, strncat 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>
> >
> > <...>
> >
> > > @@ -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,
> > > + lockfile = calloc(LOCKFILE_HOME_PATH + strlen(lockname) + 1,
> > > sizeof(char));
> > >
> > > - strcat(lockfile, home_path);
> > > - strcat(lockfile, "/.lock_nfp_secondary");
> > > + strncat(lockfile, home_path, LOCKFILE_HOME_PATH);
> > > + strncat(lockfile, lockname, strlen(lockfile));
> >
> > I guess this need to be 'LOCKFILE_HOME_PATH - strlen(lockfile) - 1' instead.
> > But also this can be implemented as 'snprintf()'
> >
> > Since 'lockfile' allocated dynamically based on sizes of existing strings, using
> > 'lockname' instead of "/.lock_nfp_secondary" will show that there won't be any
> > overflow but tools still may be complaining about 'strcat' usage.
> >
> >
>
> Why not use vasprintf() instead of doing manual construction?
Any update please?
@@ -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]);
+ strncat(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);
+ strncat(flag_str, unknown,
+ BNX2X_INFO_STR_MAX - strlen(flag_str) - 1);
}
return flag_str;
}
@@ -12175,8 +12175,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, "_");
+ strncat(name, proto[n].name,
+ sizeof(name) - strlen(name) - 1);
+ strncat(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,
+ lockfile = calloc(LOCKFILE_HOME_PATH + strlen(lockname) + 1,
sizeof(char));
- strcat(lockfile, home_path);
- strcat(lockfile, "/.lock_nfp_secondary");
+ strncat(lockfile, home_path, LOCKFILE_HOME_PATH);
+ strncat(lockfile, lockname, strlen(lockfile));
desc->secondary_lock = open(lockfile, O_RDWR | O_CREAT | O_NONBLOCK,
0666);
if (desc->secondary_lock < 0) {
@@ -374,7 +374,8 @@ testsuite_setup(void)
snprintf(vdev_args, sizeof(vdev_args),
"%s%d", temp_str, i);
strcpy(temp_str, vdev_args);
- strcat(temp_str, ";");
+ strncat(temp_str, ";",
+ VDEV_ARGS_SIZE - strlen(temp_str) - 1);
slave_core_count++;
socket_id = lcore_config[i].socket_id;
}