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.prakhya@xxxxxxxxx>
> Signed-off-by: Vikas Shivappa <vikas.shivappa@xxxxxxxxxxxxxxx>
> Tested-by: Prakhya, Sai Praneeth <sai.praneeth.prakhya@xxxxxxxxx>
> ---
>  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) {