Re: [PATCH v2] powerpc/powernv: Restrict OPAL symbol map to only be readable by root

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, May 03, 2019 at 05:52:53PM +1000, Andrew Donnellan wrote:
> Currently the OPAL symbol map is globally readable, which seems bad as it
> contains physical addresses.
> 
> Restrict it to root.
> 
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Jordan Niethe <jniethe5@gmail.com>
> Cc: Stewart Smith <stewart@linux.ibm.com>
> Fixes: c8742f85125d ("powerpc/powernv: Expose OPAL firmware symbol map")
> Cc: stable@vger.kernel.org
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
> 
> ---
> 
> v1->v2:
> 
> - fix tabs vs spaces (Greg)
> ---
>  arch/powerpc/platforms/powernv/opal.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 2b0eca104f86..0582a02623d0 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -681,7 +681,10 @@ static ssize_t symbol_map_read(struct file *fp, struct kobject *kobj,
>  				       bin_attr->size);
>  }
>  
> -static BIN_ATTR_RO(symbol_map, 0);
> +static struct bin_attribute symbol_map_attr = {
> +	.attr = {.name = "symbol_map", .mode = 0400},
> +	.read = symbol_map_read
> +};

There's no real need to rename the structure, right?  Why not just keep
the bin_attr_symbol_map name?  That would make this patch even smaller.

>  static void opal_export_symmap(void)
>  {
> @@ -698,10 +701,10 @@ static void opal_export_symmap(void)
>  		return;
>  
>  	/* Setup attributes */
> -	bin_attr_symbol_map.private = __va(be64_to_cpu(syms[0]));
> -	bin_attr_symbol_map.size = be64_to_cpu(syms[1]);
> +	symbol_map_attr.private = __va(be64_to_cpu(syms[0]));
> +	symbol_map_attr.size = be64_to_cpu(syms[1]);
>  
> -	rc = sysfs_create_bin_file(opal_kobj, &bin_attr_symbol_map);
> +	rc = sysfs_create_bin_file(opal_kobj, &symbol_map_attr);

Meta-comment, odds are you are racing userspace when you create this
sysfs file, why not add it to the device's default attributes so the
driver core creates it for you at the correct time?

thanks,

greg k-h