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 (file)
--- 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;
 
        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;
        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;
 
 {
        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
 }
 
 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) {
                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;
 
                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, " ");
 
        /* 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);
                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) {
        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 (executable)
--- 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"
 
 }
 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
 
 test_78() { #LU-6673
        local rc