Re: [PATCH 3/3] x86/intel_rdt: Return error for incorrect resource names in schemata

[Thomas Gleixner]

On Wed, 19 Apr 2017, Vikas Shivappa wrote:

> When schemata parses the resource names, currently it may not return
> error for incorrect resource names and fail quietly without updating the
> control values.
> 
> This is because for_each_enabled_rdt_resource(r) leaves "r" pointing
> beyond the end of the rdt_resources_all[] array, and we check for
> !r->name which results in an out of bounds access and also may not be
> NULL, hence we may not detect that we did not find the name user
> supplied.
> 
> Update this to check (r == (rdt_resources_all + RDT_NUM_RESOURCES))
> instead.
> 
> Reported-by: Prakhya, Sai Praneeth <sai.praneeth.prak...@intel.com>
> Signed-off-by: Vikas Shivappa <vikas.shiva...@linux.intel.com>
> Tested-by: Prakhya, Sai Praneeth <sai.praneeth.prak...@intel.com>
> ---
>  arch/x86/kernel/cpu/intel_rdt_schemata.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c 
> b/arch/x86/kernel/cpu/intel_rdt_schemata.c
> index 3cfa1ca..bb99bd8 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
> @@ -228,7 +228,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file 
> *of,
>                               break;
>                       }
>               }
> -             if (!r->name) {
> +             if (r == (rdt_resources_all + RDT_NUM_RESOURCES)) {
>                       ret = -EINVAL;
>                       goto out;
>               }

The resulting loop is just horrible to read. We can do better than
that. Patch below.

Thanks,

        tglx
8<-------------

--- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -187,6 +187,17 @@ static int update_domains(struct rdt_res
        return 0;
 }
 
+static int rdtgroup_parse_resource(char *resname, char *tok, int closid)
+{
+       struct rdt_resource *r;
+
+       for_each_enabled_rdt_resource(r) {
+               if (!strcmp(resname, r->name) && closid < r->num_closid)
+                       return parse_line(tok, r);
+       }
+       return -EINVAL;
+}
+
 ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
                                char *buf, size_t nbytes, loff_t off)
 {
@@ -209,9 +220,10 @@ ssize_t rdtgroup_schemata_write(struct k
 
        closid = rdtgrp->closid;
 
-       for_each_enabled_rdt_resource(r)
+       for_each_enabled_rdt_resource(r) {
                list_for_each_entry(dom, &r->domains, list)
                        dom->have_new_ctrl = false;
+       }
 
        while ((tok = strsep(&buf, "\n")) != NULL) {
                resname = strsep(&tok, ":");
@@ -219,19 +231,9 @@ ssize_t rdtgroup_schemata_write(struct k
                        ret = -EINVAL;
                        goto out;
                }
-               for_each_enabled_rdt_resource(r) {
-                       if (!strcmp(strim(resname), r->name) &&
-                           closid < r->num_closid) {
-                               ret = parse_line(tok, r);
-                               if (ret)
-                                       goto out;
-                               break;
-                       }
-               }
-               if (!r->name) {
-                       ret = -EINVAL;
+               ret = rdtgroup_parse_resource(strim(resname), tok, closid);
+               if (ret)
                        goto out;
-               }
        }
 
        for_each_enabled_rdt_resource(r) {