Message-ID: <574522c3-3fc3-2c89-dfeb-587b9d7cd32c@linux.ibm.com>
Date:   Tue, 31 Jan 2023 10:54:07 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH v5 11/25] powerpc/secvar: Allow backend to populate static
 list of variable names
Content-Language: en-US
To:     Andrew Donnellan <ajd@linux.ibm.com>,
        linuxppc-dev@lists.ozlabs.org, linux-integrity@vger.kernel.org
Cc:     ruscur@russell.cc, bgray@linux.ibm.com, nayna@linux.ibm.com,
        gcwilson@linux.ibm.com, gjoyce@linux.ibm.com, brking@linux.ibm.com,
        sudhakar@linux.ibm.com, erichte@linux.ibm.com,
        gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
        zohar@linux.ibm.com, joel@jms.id.au, npiggin@gmail.com
References: <20230131063928.388035-1-ajd@linux.ibm.com>
 <20230131063928.388035-12-ajd@linux.ibm.com>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <20230131063928.388035-12-ajd@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/31/23 01:39, Andrew Donnellan wrote:
> Currently, the list of variables is populated by calling
> secvar_ops->get_next() repeatedly, which is explicitly modelled on the
> OPAL API (including the keylen parameter).
> 
> For the upcoming PLPKS backend, we have a static list of variable names.
> It is messy to fit that into get_next(), so instead, let the backend put
> a NULL-terminated array of variable names into secvar_ops->var_names,
> which will be used if get_next() is undefined.
> 
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> 
> ---
> 
> v3: New patch (ajd/mpe)
> ---
>   arch/powerpc/include/asm/secvar.h  |  4 ++
>   arch/powerpc/kernel/secvar-sysfs.c | 67 ++++++++++++++++++++----------
>   2 files changed, 50 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/secvar.h b/arch/powerpc/include/asm/secvar.h
> index 011a53a8076c..4828e0ab7e3c 100644
> --- a/arch/powerpc/include/asm/secvar.h
> +++ b/arch/powerpc/include/asm/secvar.h
> @@ -21,6 +21,10 @@ struct secvar_operations {
>   	ssize_t (*format)(char *buf, size_t bufsize);
>   	int (*max_size)(u64 *max_size);
>   	const struct attribute **config_attrs;
> +
> +	// NULL-terminated array of fixed variable names
> +	// Only used if get_next() isn't provided
> +	const char * const *var_names;
>   };
>   
>   #ifdef CONFIG_PPC_SECURE_BOOT
> diff --git a/arch/powerpc/kernel/secvar-sysfs.c b/arch/powerpc/kernel/secvar-sysfs.c
> index 7df32be86507..2cbc60b37e4e 100644
> --- a/arch/powerpc/kernel/secvar-sysfs.c
> +++ b/arch/powerpc/kernel/secvar-sysfs.c
> @@ -157,9 +157,31 @@ static int secvar_sysfs_config(struct kobject *kobj)
>   	return 0;
>   }
>   
> -static int secvar_sysfs_load(void)
> +static int add_var(const char *name)
>   {
>   	struct kobject *kobj;
> +	int rc;
> +
> +	kobj = kzalloc(sizeof(*kobj), GFP_KERNEL);
> +	if (!kobj)
> +		return -ENOMEM;
> +
> +	kobject_init(kobj, &secvar_ktype);
> +
> +	rc = kobject_add(kobj, &secvar_kset->kobj, "%s", name);
> +	if (rc) {
> +		pr_warn("kobject_add error %d for attribute: %s\n", rc,
> +			name);
> +		kobject_put(kobj);
> +		return rc;
> +	}
> +
> +	kobject_uevent(kobj, KOBJ_ADD);
> +	return 0;
> +}
> +
> +static int secvar_sysfs_load(void)
> +{
>   	u64 namesize = 0;
>   	char *name;
>   	int rc;
> @@ -179,31 +201,26 @@ static int secvar_sysfs_load(void)
>   			break;
>   		}
>   
> -		kobj = kzalloc(sizeof(*kobj), GFP_KERNEL);
> -		if (!kobj) {
> -			rc = -ENOMEM;
> -			break;
> -		}
> -
> -		kobject_init(kobj, &secvar_ktype);
> -
> -		rc = kobject_add(kobj, &secvar_kset->kobj, "%s", name);
> -		if (rc) {
> -			pr_warn("kobject_add error %d for attribute: %s\n", rc,
> -				name);
> -			kobject_put(kobj);
> -			kobj = NULL;
> -		}
> -
> -		if (kobj)
> -			kobject_uevent(kobj, KOBJ_ADD);
> -
> +		rc = add_var(name);
>   	} while (!rc);
>   
>   	kfree(name);
>   	return rc;
>   }
>   
> +static int secvar_sysfs_load_static(void)
> +{
> +	const char * const *name_ptr = secvar_ops->var_names;
> +	int rc;

Missing newline ?

> +	while (*name_ptr) {
> +		rc = add_var(*name_ptr);
> +		if (rc)
> +			return rc;
> +		name_ptr++;
> +	}
> +	return 0;
> +}
> +
>   static int secvar_sysfs_init(void)
>   {
>   	int rc;
> @@ -245,7 +262,15 @@ static int secvar_sysfs_init(void)
>   		goto err;
>   	}
>   
> -	secvar_sysfs_load();
> +	if (secvar_ops->get_next)
> +		rc = secvar_sysfs_load();
> +	else
> +		rc = secvar_sysfs_load_static();
> +
> +	if (rc) {
> +		pr_err("Failed to create variable attributes\n");
> +		goto err;
> +	}
>   
>   	return 0;
>   err:

With the above change:

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>