diff mbox series

[v2] drivers: fix to replace strcat with strncat

Message ID 1547825033-3595-1-git-send-email-tallurix.chaitanya.babu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] drivers: fix to replace strcat with strncat |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Chaitanya Babu, TalluriX Jan. 18, 2019, 3:23 p.m. UTC 
  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>
---
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 | 8 +++++---
 test/test/test_cryptodev.c                 | 3 ++-
 4 files changed, 15 insertions(+), 8 deletions(-)

Comments

Jananee Parthasarathy Jan. 21, 2019, 10:43 a.m. UTC | #1 
>-----Original Message-----
>From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chaitanya Babu Talluri
>Sent: Friday, January 18, 2019 8:54 PM
>To: dev@dpdk.org
>Cc: rmody@marvell.com; Pattan, Reshma <reshma.pattan@intel.com>;
>shshaikh@marvell.com; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
><qi.z.zhang@intel.com>; alejandro.lucero@netronome.com; De Lara Guarch,
>Pablo <pablo.de.lara.guarch@intel.com>; Doherty, Declan
><declan.doherty@intel.com>; Chaitanya Babu, TalluriX
><tallurix.chaitanya.babu@intel.com>; stable@dpdk.org
>Subject: [dpdk-dev] [PATCH v2] drivers: fix to replace strcat with strncat
>
>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>
>---
>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 | 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;
> }
>diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
>index 8dc1a4af8..56867ff84 100644
>--- a/drivers/net/i40e/i40e_ethdev.c
>+++ b/drivers/net/i40e/i40e_ethdev.c
>@@ -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;
> 			}
> 		}
>diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
>b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
>index 39bd48a83..a9c727185 100644
>--- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
>+++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
>@@ -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");
>+	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) {
>diff --git a/test/test/test_cryptodev.c b/test/test/test_cryptodev.c index
>84065eb49..a979603b9 100644
>--- a/test/test/test_cryptodev.c
>+++ b/test/test/test_cryptodev.c
>@@ -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;
> 		}
>--
>2.17.2

Any Review Please!!
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
Ferruh Yigit Feb. 7, 2019, 11:56 a.m. UTC | #2 
On 1/21/2019 10:43 AM, Parthasarathy, JananeeX M wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chaitanya Babu Talluri
>> Sent: Friday, January 18, 2019 8:54 PM
>> To: dev@dpdk.org
>> Cc: rmody@marvell.com; Pattan, Reshma <reshma.pattan@intel.com>;
>> shshaikh@marvell.com; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
>> <qi.z.zhang@intel.com>; alejandro.lucero@netronome.com; De Lara Guarch,
>> Pablo <pablo.de.lara.guarch@intel.com>; Doherty, Declan
>> <declan.doherty@intel.com>; Chaitanya Babu, TalluriX
>> <tallurix.chaitanya.babu@intel.com>; stable@dpdk.org
>> Subject: [dpdk-dev] [PATCH v2] drivers: fix to replace strcat with strncat
>>
>> 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>

<...>

> 
> Any Review Please!!
> 

cc'ed Bruce & Thomas.

What do you think getting strlcat() patch first and updating this patch to use
strlcat()?

Are we OK to get strlcat as the default API?

Thanks,
ferruh
Thomas Monjalon Feb. 7, 2019, 12:08 p.m. UTC | #3 
07/02/2019 12:56, Ferruh Yigit:
> On 1/21/2019 10:43 AM, Parthasarathy, JananeeX M wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chaitanya Babu Talluri
> >> Sent: Friday, January 18, 2019 8:54 PM
> >> To: dev@dpdk.org
> >> Cc: rmody@marvell.com; Pattan, Reshma <reshma.pattan@intel.com>;
> >> shshaikh@marvell.com; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
> >> <qi.z.zhang@intel.com>; alejandro.lucero@netronome.com; De Lara Guarch,
> >> Pablo <pablo.de.lara.guarch@intel.com>; Doherty, Declan
> >> <declan.doherty@intel.com>; Chaitanya Babu, TalluriX
> >> <tallurix.chaitanya.babu@intel.com>; stable@dpdk.org
> >> Subject: [dpdk-dev] [PATCH v2] drivers: fix to replace strcat with strncat
> >>
> >> 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>
> 
> <...>
> 
> > 
> > Any Review Please!!
> > 
> 
> cc'ed Bruce & Thomas.
> 
> What do you think getting strlcat() patch first and updating this patch to use
> strlcat()?
> 
> Are we OK to get strlcat as the default API?

No problem
Bruce Richardson Feb. 7, 2019, 1:27 p.m. UTC | #4 
On Thu, Feb 07, 2019 at 11:56:30AM +0000, Ferruh Yigit wrote:
> On 1/21/2019 10:43 AM, Parthasarathy, JananeeX M wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chaitanya Babu Talluri
> >> Sent: Friday, January 18, 2019 8:54 PM
> >> To: dev@dpdk.org
> >> Cc: rmody@marvell.com; Pattan, Reshma <reshma.pattan@intel.com>;
> >> shshaikh@marvell.com; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
> >> <qi.z.zhang@intel.com>; alejandro.lucero@netronome.com; De Lara Guarch,
> >> Pablo <pablo.de.lara.guarch@intel.com>; Doherty, Declan
> >> <declan.doherty@intel.com>; Chaitanya Babu, TalluriX
> >> <tallurix.chaitanya.babu@intel.com>; stable@dpdk.org
> >> Subject: [dpdk-dev] [PATCH v2] drivers: fix to replace strcat with strncat
> >>
> >> 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>
> 
> <...>
> 
> > 
> > Any Review Please!!
> > 
> 
> cc'ed Bruce & Thomas.
> 
> What do you think getting strlcat() patch first and updating this patch to use
> strlcat()?
> 
> Are we OK to get strlcat as the default API?
> 
"strlcat" is just saner to use, so +1 for this approach.

/Bruce
Ferruh Yigit Feb. 13, 2019, 11:54 a.m. UTC | #5 
On 2/7/2019 1:27 PM, Bruce Richardson wrote:
> On Thu, Feb 07, 2019 at 11:56:30AM +0000, Ferruh Yigit wrote:
>> On 1/21/2019 10:43 AM, Parthasarathy, JananeeX M wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chaitanya Babu Talluri
>>>> Sent: Friday, January 18, 2019 8:54 PM
>>>> To: dev@dpdk.org
>>>> Cc: rmody@marvell.com; Pattan, Reshma <reshma.pattan@intel.com>;
>>>> shshaikh@marvell.com; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
>>>> <qi.z.zhang@intel.com>; alejandro.lucero@netronome.com; De Lara Guarch,
>>>> Pablo <pablo.de.lara.guarch@intel.com>; Doherty, Declan
>>>> <declan.doherty@intel.com>; Chaitanya Babu, TalluriX
>>>> <tallurix.chaitanya.babu@intel.com>; stable@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH v2] drivers: fix to replace strcat with strncat
>>>>
>>>> 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>
>>
>> <...>
>>
>>>
>>> Any Review Please!!
>>>
>>
>> cc'ed Bruce & Thomas.
>>
>> What do you think getting strlcat() patch first and updating this patch to use
>> strlcat()?
>>
>> Are we OK to get strlcat as the default API?
>>
> "strlcat" is just saner to use, so +1 for this approach.

Hi Jananee,

'strlcat' support is merged into main repo now. Can you please send a new
version of this patch to use 'strlcat'?

Thanks,
ferruh
diff mbox series

Patch

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;
 }
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 8dc1a4af8..56867ff84 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -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;
 			}
 		}
diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
index 39bd48a83..a9c727185 100644
--- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
+++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
@@ -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");
+	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) {
diff --git a/test/test/test_cryptodev.c b/test/test/test_cryptodev.c
index 84065eb49..a979603b9 100644
--- a/test/test/test_cryptodev.c
+++ b/test/test/test_cryptodev.c
@@ -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;
 		}