[PATCH] net/cpfl: refactor flow parser

[PATCH] net/cpfl: refactor flow parser

[wenjing.qiao]

From: Wenjing Qiao <wenjing.qiao@intel.com>

Use strncpy instead of memcpy for string copy and rename macro.

Coverity issue: 405350
Fixes: 6cc97c9971d7 ("net/cpfl: build action mapping rules from JSON")

Signed-off-by: Wenjing Qiao <wenjing.qiao@intel.com>
---
 drivers/net/cpfl/cpfl_flow_parser.c | 34 +++++++++++++++------
 drivers/net/cpfl/cpfl_flow_parser.h | 46 ++++++++++++++---------------
 2 files changed, 48 insertions(+), 32 deletions(-)

diff --git a/drivers/net/cpfl/cpfl_flow_parser.c b/drivers/net/cpfl/cpfl_flow_parser.c
index 1e0ba289c2..a8f0488f21 100644
--- a/drivers/net/cpfl/cpfl_flow_parser.c
+++ b/drivers/net/cpfl/cpfl_flow_parser.c
@@ -205,11 +205,11 @@ cpfl_flow_js_pattern_key_proto_field(json_t *ob_fields,
 			PMD_DRV_LOG(ERR, "Can not parse string 'name'.");
 			goto err;
 		}
-		if (strlen(name) > CPFL_FLOW_JSON_STR_SIZE_MAX) {
+		if (strlen(name) > CPFL_JS_STR_SIZE - 1) {
 			PMD_DRV_LOG(ERR, "The 'name' is too long.");
 			goto err;
 		}
-		memcpy(js_field->fields[i].name, name, strlen(name));
+		strncpy(js_field->fields[i].name, name, CPFL_JS_STR_SIZE - 1);
 
 		if (js_field->type == RTE_FLOW_ITEM_TYPE_ETH ||
 		    js_field->type == RTE_FLOW_ITEM_TYPE_IPV4) {
@@ -218,11 +218,11 @@ cpfl_flow_js_pattern_key_proto_field(json_t *ob_fields,
 				PMD_DRV_LOG(ERR, "Can not parse string 'mask'.");
 				goto err;
 			}
-			if (strlen(mask) > CPFL_FLOW_JSON_STR_SIZE_MAX) {
+			if (strlen(mask) > CPFL_JS_STR_SIZE - 1) {
 				PMD_DRV_LOG(ERR, "The 'mask' is too long.");
 				goto err;
 			}
-			memcpy(js_field->fields[i].mask, mask, strlen(mask));
+			strncpy(js_field->fields[i].mask, mask, CPFL_JS_STR_SIZE - 1);
 		} else {
 			uint32_t mask_32b;
 			int ret;
@@ -633,8 +633,12 @@ cpfl_flow_js_mr_key(json_t *ob_mr_keys, struct cpfl_flow_js_mr_key *js_mr_key)
 					PMD_DRV_LOG(ERR, "Can not parse string 'name'.");
 					goto err;
 				}
+				if (strlen(name) > CPFL_JS_STR_SIZE - 1) {
+					PMD_DRV_LOG(ERR, "The 'name' is too long.");
+					goto err;
+				}
 				strncpy(js_mr_key->actions[i].prog.name, name,
-					CPFL_FLOW_JSON_STR_SIZE_MAX - 1);
+					CPFL_JS_STR_SIZE - 1);
 			}
 
 			ob_param = json_object_get(object, "parameters");
@@ -655,8 +659,12 @@ cpfl_flow_js_mr_key(json_t *ob_mr_keys, struct cpfl_flow_js_mr_key *js_mr_key)
 						PMD_DRV_LOG(ERR, "Can not parse string 'name'.");
 						goto err;
 					}
+					if (strlen(name) > CPFL_JS_STR_SIZE - 1) {
+						PMD_DRV_LOG(ERR, "The 'name' is too long.");
+						goto err;
+					}
 					strncpy(js_mr_key->actions[i].prog.params[j].name, name,
-						CPFL_FLOW_JSON_STR_SIZE_MAX - 1);
+						CPFL_JS_STR_SIZE - 1);
 				}
 				ret = cpfl_json_t_to_uint16(subobject, "size", &value);
 				if (ret < 0) {
@@ -719,7 +727,11 @@ cpfl_flow_js_mr_layout(json_t *ob_layouts, struct cpfl_flow_js_mr_action_mod *js
 			PMD_DRV_LOG(ERR, "Can not parse string 'hint'.");
 			goto err;
 		}
-		memcpy(js_mod->layout[i].hint, hint, strlen(hint));
+		if (strlen(hint) > CPFL_JS_STR_SIZE - 1) {
+			PMD_DRV_LOG(ERR, "The 'hint' is too long.");
+			goto err;
+		}
+		strncpy(js_mod->layout[i].hint, hint, CPFL_JS_STR_SIZE - 1);
 	}
 
 	return 0;
@@ -762,7 +774,11 @@ cpfl_flow_js_mr_content(json_t *ob_content, struct cpfl_flow_js_mr_action_mod *j
 			PMD_DRV_LOG(ERR, "Can not parse string 'type'.");
 			goto err;
 		}
-		strncpy(js_mod->content.fields[i].type, type, CPFL_FLOW_JSON_STR_SIZE_MAX - 1);
+		if (strlen(type) > CPFL_JS_STR_SIZE - 1) {
+			PMD_DRV_LOG(ERR, "The 'type' is too long.");
+			goto err;
+		}
+		strncpy(js_mod->content.fields[i].type, type, CPFL_JS_STR_SIZE - 1);
 		ret = cpfl_json_t_to_uint16(object, "start", &start);
 		if (ret < 0) {
 			PMD_DRV_LOG(ERR, "Can not parse 'start'.");
@@ -1698,7 +1714,7 @@ cpfl_parse_check_prog_action(struct cpfl_flow_js_mr_key_action *key_act,
 		if (param->has_name) {
 			mr_key_prog->has_name = TRUE;
 			strncpy(mr_key_prog->name[param->index], param->name,
-				CPFL_FLOW_JSON_STR_SIZE_MAX - 1);
+				CPFL_JS_STR_SIZE - 1);
 		}
 	}
 
diff --git a/drivers/net/cpfl/cpfl_flow_parser.h b/drivers/net/cpfl/cpfl_flow_parser.h
index c9a9772f13..23904e39f1 100644
--- a/drivers/net/cpfl/cpfl_flow_parser.h
+++ b/drivers/net/cpfl/cpfl_flow_parser.h
@@ -9,13 +9,13 @@
 #ifndef _CPFL_FLOW_PARSER_H_
 #define _CPFL_FLOW_PARSER_H_
 
-#define CPFL_FLOW_JSON_STR_SIZE_MAX 100
-#define CPFL_MAX_SEM_FV_KEY_SIZE 64
-#define CPFL_FLOW_JS_PROTO_SIZE 16
-#define CPFL_MOD_KEY_NUM_MAX 8
-#define CPFL_PROG_CONTENT_FIELD_NUM_MAX 64
-#define CPFL_PROG_CONSTANT_VALUE_NUM_MAX 8
-#define CPFL_PROG_PARAM_NUM_MAX 10
+#define CPFL_JS_STR_SIZE 100
+#define CPFL_JS_SEM_FV_KEY_NUM_MAX 64
+#define CPFL_JS_PROTO_NUM_MAX 16
+#define CPFL_JS_MOD_KEY_NUM_MAX 8
+#define CPFL_JS_PROG_CONTENT_FIELD_NUM_MAX 64
+#define CPFL_JS_PROG_CONSTANT_VALUE_NUM_MAX 8
+#define CPFL_JS_PROG_PARAM_NUM_MAX 10
 
 /* Pattern Rules Storage */
 enum cpfl_flow_pr_action_type {
@@ -30,9 +30,9 @@ struct cpfl_flow_js_pr_key_attr {
 };
 
 struct cpfl_flow_js_pr_key_proto_field {
-	char name[CPFL_FLOW_JSON_STR_SIZE_MAX];
+	char name[CPFL_JS_STR_SIZE];
 	union {
-		char mask[CPFL_FLOW_JSON_STR_SIZE_MAX];
+		char mask[CPFL_JS_STR_SIZE];
 		uint32_t mask_32b;
 	};
 };
@@ -80,7 +80,7 @@ struct cpfl_flow_js_fv {
 struct cpfl_flow_js_pr_action_sem {
 	uint16_t prof;		    /* SEM profile ID */
 	uint16_t subprof;	    /* SEM subprofile ID */
-	uint16_t keysize;	    /*  extract key size in bytes */
+	uint16_t keysize;	    /* extract key size in bytes */
 	struct cpfl_flow_js_fv *fv; /* A SEM field vector array */
 	int fv_size;
 };
@@ -116,23 +116,23 @@ struct cpfl_flow_js_pr {
  * of data.
  */
 struct cpfl_flow_js_mr_key_action_vxlan_encap {
-	enum rte_flow_item_type protocols[CPFL_FLOW_JS_PROTO_SIZE];
+	enum rte_flow_item_type protocols[CPFL_JS_PROTO_NUM_MAX];
 	int proto_size;
 };
 
 struct cpfl_flow_js_prog_parameter {
 	bool has_name;
 	uint16_t index;
-	char name[CPFL_FLOW_JSON_STR_SIZE_MAX];
+	char name[CPFL_JS_STR_SIZE];
 	uint16_t size;
 };
 
 struct cpfl_flow_js_mr_key_action_prog {
 	bool has_name;
 	uint32_t id;
-	char name[CPFL_FLOW_JSON_STR_SIZE_MAX];
+	char name[CPFL_JS_STR_SIZE];
 	uint32_t param_size;
-	struct cpfl_flow_js_prog_parameter params[CPFL_PROG_PARAM_NUM_MAX];
+	struct cpfl_flow_js_prog_parameter params[CPFL_JS_PROG_PARAM_NUM_MAX];
 };
 
 /* A set of modification rte_flow_action_xxx objects can be defined as a type / data pair. */
@@ -151,24 +151,24 @@ struct cpfl_flow_js_mr_key {
 
 struct cpfl_flow_js_mr_layout {
 	int index;				/* links to the element of the actions array */
-	char hint[CPFL_FLOW_JSON_STR_SIZE_MAX]; /* where the data to copy from */
+	char hint[CPFL_JS_STR_SIZE]; /* where the data to copy from */
 	uint16_t offset;			/* the start byte of the data to copy from */
 	uint16_t size; /*  bytes of the data to be copied to the memory region */
 };
 
 struct cpfl_flow_js_mr_field {
-	char type[CPFL_FLOW_JSON_STR_SIZE_MAX];
+	char type[CPFL_JS_STR_SIZE];
 	uint16_t start;
 	uint16_t width;
 	union {
 		uint16_t index;
-		uint8_t value[CPFL_PROG_CONSTANT_VALUE_NUM_MAX];
+		uint8_t value[CPFL_JS_PROG_CONSTANT_VALUE_NUM_MAX];
 	};
 };
 
 struct cpfl_flow_js_mr_content {
 	uint16_t size;
-	struct cpfl_flow_js_mr_field fields[CPFL_PROG_CONTENT_FIELD_NUM_MAX];
+	struct cpfl_flow_js_mr_field fields[CPFL_JS_PROG_CONTENT_FIELD_NUM_MAX];
 	int field_size;
 };
 
@@ -182,7 +182,7 @@ struct cpfl_flow_js_mr_action_mod {
 	bool is_content;
 	union {
 		struct {
-			struct cpfl_flow_js_mr_layout layout[CPFL_FLOW_JS_PROTO_SIZE];
+			struct cpfl_flow_js_mr_layout layout[CPFL_JS_PROTO_NUM_MAX];
 			int layout_size;
 		};
 		struct cpfl_flow_js_mr_content content;
@@ -227,7 +227,7 @@ struct cpfl_flow_pr_action_sem {
 	uint16_t prof;
 	uint16_t subprof;
 	uint16_t keysize;
-	uint8_t cpfl_flow_pr_fv[CPFL_MAX_SEM_FV_KEY_SIZE];
+	uint8_t cpfl_flow_pr_fv[CPFL_JS_SEM_FV_KEY_NUM_MAX];
 };
 
 struct cpfl_flow_pr_action {
@@ -239,7 +239,7 @@ struct cpfl_flow_pr_action {
 
 /* Modification Rules */
 struct cpfl_flow_mr_key_action_vxlan_encap {
-	enum rte_flow_item_type protocols[CPFL_FLOW_JS_PROTO_SIZE];
+	enum rte_flow_item_type protocols[CPFL_JS_PROTO_NUM_MAX];
 	uint16_t proto_size;
 	const struct rte_flow_action *action;
 };
@@ -247,7 +247,7 @@ struct cpfl_flow_mr_key_action_vxlan_encap {
 struct cpfl_flow_mr_key_action_prog {
 	const struct rte_flow_action_prog *prog;
 	bool has_name;
-	char name[CPFL_PROG_PARAM_NUM_MAX][CPFL_FLOW_JSON_STR_SIZE_MAX];
+	char name[CPFL_JS_PROG_PARAM_NUM_MAX][CPFL_JS_STR_SIZE];
 };
 
 struct cpfl_flow_mr_key_mod {
@@ -256,7 +256,7 @@ struct cpfl_flow_mr_key_mod {
 };
 
 struct cpfl_flow_mr_key_action {
-	struct cpfl_flow_mr_key_mod mods[CPFL_MOD_KEY_NUM_MAX];
+	struct cpfl_flow_mr_key_mod mods[CPFL_JS_MOD_KEY_NUM_MAX];
 	struct cpfl_flow_mr_key_action_prog prog;
 };
 
-- 
2.34.1

RE: [PATCH] net/cpfl: refactor flow parser

[Zhang, Qi Z]

> -----Original Message-----
> From: wenjing.qiao@intel.com <wenjing.qiao@intel.com>
> Sent: Tuesday, November 7, 2023 4:35 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Qiao, Wenjing <wenjing.qiao@intel.com>
> Subject: [PATCH] net/cpfl: refactor flow parser
> 
> From: Wenjing Qiao <wenjing.qiao@intel.com>
> 
> Use strncpy instead of memcpy for string copy and rename macro.
> 
> Coverity issue: 405350
> Fixes: 6cc97c9971d7 ("net/cpfl: build action mapping rules from JSON")
> 
> Signed-off-by: Wenjing Qiao <wenjing.qiao@intel.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

Re: [PATCH] net/cpfl: refactor flow parser

[Thomas Monjalon]

07/11/2023 09:34, wenjing.qiao@intel.com:
> From: Wenjing Qiao <wenjing.qiao@intel.com>
> 
> Use strncpy instead of memcpy for string copy and rename macro.

I really wonder why memcpy was used for string copy.
As you fix it, why not using strlcpy, as recommended by checkpatch?

Qi, why do you keep ignoring warnings reported in patchwork?

RE: [PATCH] net/cpfl: refactor flow parser

[Zhang, Qi Z]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, November 13, 2023 1:39 AM
> To: Qiao, Wenjing <wenjing.qiao@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> dev@dpdk.org; Marchand, David <david.marchand@redhat.com>
> Subject: Re: [PATCH] net/cpfl: refactor flow parser
> 
> 07/11/2023 09:34, wenjing.qiao@intel.com:
> > From: Wenjing Qiao <wenjing.qiao@intel.com>
> >
> > Use strncpy instead of memcpy for string copy and rename macro.
> 
> I really wonder why memcpy was used for string copy.
> As you fix it, why not using strlcpy, as recommended by checkpatch?
> 
> Qi, why do you keep ignoring warnings reported in patchwork?

Have to nack this, there is no related patchwork error and warning was reported for the original patches.
https://patchwork.dpdk.org/project/dpdk/list/?series=29767&state=*

And I agree strlcpy is better than strncpy , thanks for the suggestion.




>