From: Etienne AUJAMES <eaujames@ddn.com>
Date: Mon, 4 Oct 2021 18:42:31 +0000 (+0200)
Subject: LU-15056 nrs: length of a tbf rule should be checked
X-Git-Tag: 2.14.57~74
X-Git-Url: https://git.whamcloud.com/?p=fs%2Flustre-release.git;a=commitdiff_plain;h=1937c6cd75a2346b353f4cb6aacda8d0b6df804d

LU-15056 nrs: length of a tbf rule should be checked

The maximum size of a tbf rule name is 16 bytes (MAX_TBF_NAME). This
length is not verify before applying the rule. This causes a buffer
overflow at name copy.

This patch adds a str length verification inside name_is_invalid().
The test sanityn 77p checks if an error is returned when user try to
register a rule with an invalid name.

Signed-off-by: Etienne AUJAMES <eaujames@ddn.com>
Change-Id: I93c73083b6e81ab9070a860e702e56b0cb498352
Reviewed-on: https://review.whamcloud.com/45124
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Li Xi <lixi@ddn.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>
---

diff --git a/lustre/ptlrpc/nrs_tbf.c b/lustre/ptlrpc/nrs_tbf.c
index 47724dc..37edf58 100644
--- a/lustre/ptlrpc/nrs_tbf.c
+++ b/lustre/ptlrpc/nrs_tbf.c
@@ -297,7 +297,7 @@ nrs_tbf_rule_start(struct ptlrpc_nrs_policy *policy,
 	if (rule == NULL)
 		return -ENOMEM;
 
-	memcpy(rule->tr_name, start->tc_name, strlen(start->tc_name));
+	strlcpy(rule->tr_name, start->tc_name, sizeof(rule->tr_name));
 	rule->tr_rpc_rate = start->u.tc_start.ts_rpc_rate;
 	rule->tr_flags = start->u.tc_start.ts_rule_flags;
 	rule->tr_nsecs_per_rpc = NSEC_PER_SEC / rule->tr_rpc_rate;
@@ -3378,16 +3378,22 @@ static void nrs_tbf_cmd_fini(struct nrs_tbf_cmd *cmd)
 	}
 }
 
-static bool name_is_valid(const char *name)
+static int check_rule_name(const char *name)
 {
 	int i;
 
-	for (i = 0; i < strlen(name); i++) {
-		if ((!isalnum(name[i])) &&
-		    (name[i] != '_'))
-			return false;
+	if (name[0] == '\0')
+		return -EINVAL;
+
+	for (i = 0; name[i] != '\0' && i < MAX_TBF_NAME; i++) {
+		if (!isalnum(name[i]) && name[i] != '_')
+			return -EINVAL;
 	}
-	return true;
+
+	if (i == MAX_TBF_NAME)
+		return -ENAMETOOLONG;
+
+	return 0;
 }
 
 static int
@@ -3419,8 +3425,9 @@ nrs_tbf_parse_value_pair(struct nrs_tbf_cmd *cmd, char *buffer)
 		else
 			return -EINVAL;
 	}  else if (strcmp(key, "rank") == 0) {
-		if (!name_is_valid(val))
-			return -EINVAL;
+		rc = check_rule_name(val);
+		if (rc)
+			return rc;
 
 		if (cmd->tc_cmd == NRS_CTL_TBF_START_RULE)
 			cmd->u.tc_start.ts_next_name = val;
@@ -3507,9 +3514,13 @@ nrs_tbf_parse_cmd(char *buffer, unsigned long count, __u32 type_flag)
 
 	/* Name of the rule */
 	token = strsep(&val, " ");
-	if ((val == NULL && cmd->tc_cmd != NRS_CTL_TBF_STOP_RULE) ||
-	    !name_is_valid(token))
+	if ((val == NULL && cmd->tc_cmd != NRS_CTL_TBF_STOP_RULE))
 		GOTO(out_free_cmd, rc = -EINVAL);
+
+	rc = check_rule_name(token);
+	if (rc)
+		GOTO(out_free_cmd, rc);
+
 	cmd->tc_name = token;
 
 	if (cmd->tc_cmd == NRS_CTL_TBF_START_RULE) {
diff --git a/lustre/tests/sanityn.sh b/lustre/tests/sanityn.sh
index 2cbb21f..351a346 100755
--- a/lustre/tests/sanityn.sh
+++ b/lustre/tests/sanityn.sh
@@ -4459,6 +4459,35 @@ test_77q() {
 }
 run_test 77q "Parallel TBF rule definitions should not panic"
 
+test_77p() {
+	local c
+	local -a spec_chars=(
+		'@' '.' '~' '#' '/' '^' '%' '*' ';' ',' '?' '<' '>' ':'
+		'+' '=' ')' '(' '{' '}' '|' '[' ']' '!' '&' '\$' '\`' '\\')
+
+	(( $MDS1_VERSION > $(version_code 2.14.54) )) ||
+		skip "need MDS >= 2.14.54"
+
+	do_facet mds1 $LCTL set_param mds.MDS.mdt.nrs_policies="tbf"
+	stack_trap "do_facet mds1 $LCTL set_param mds.MDS.mdt.nrs_policies=fifo"
+
+	# TBF rule name size is 16 bytes
+	do_facet mds1 $LCTL set_param mds.MDS.mdt.nrs_tbf_rule="start\ test_77p_overflo\ uid={500}\ rate=500" &&
+		error "The length of tbf rule name is not checked" || true
+	do_facet mds1 $LCTL set_param mds.MDS.mdt.nrs_tbf_rule="start\ \ uid={500}\ rate=500" &&
+		error "The server should not accept empty tbf rule name" || true
+	do_facet mds1 $LCTL set_param mds.MDS.mdt.nrs_tbf_rule="start\ test_77p_empty" &&
+		error "The server should not accept 'start <tbf_rule_name>' without an expression" || true
+
+	# Test with special chars
+	for c in "${spec_chars[@]}"; do
+		do_facet mds1 $LCTL set_param mds.MDS.mdt.nrs_tbf_rule="'start test77p${c}spec uid={500} rate=500'" &&
+		error "Special char '${c}' should not be accepted in a tbf rule name" || true
+	done
+
+}
+run_test 77p "Check validity of rule names for TBF policies"
+
 test_78() { #LU-6673
 	local rc