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

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

[Andrew Donnellan]

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
+};
 
 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);
 	if (rc)
 		pr_warn("Error %d creating OPAL symbols file\n", rc);
 }
-- 
2.20.1

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

[Greg KH]

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

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

[Andrew Donnellan]

On 3/5/19 5:59 pm, Greg KH wrote:>> -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.

No real need but it's locally more consistent with the rest of the PPC 
code. (Though perhaps the other cases should use the BIN_ATTR macro...)

Given this is for stable I'm happy to change that if the smaller patch 
is more acceptable.

> 
>>   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?

I was not previously aware of default attributes...

Are we actually racing against userspace in a subsys initcall?

-- 
Andrew Donnellan              OzLabs, ADL Canberra
ajd@linux.ibm.com             IBM Australia Limited

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

[Greg KH]

On Fri, May 03, 2019 at 06:27:18PM +1000, Andrew Donnellan wrote:
> On 3/5/19 5:59 pm, Greg KH wrote:>> -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.
> 
> No real need but it's locally more consistent with the rest of the PPC code.
> (Though perhaps the other cases should use the BIN_ATTR macro...)
> 
> Given this is for stable I'm happy to change that if the smaller patch is
> more acceptable.

stable doesn't care, and if this is more consistent, that's fine with
me, I didn't see the larger picture here, just providing unsolicited
patch review :)

> > >   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?
> 
> I was not previously aware of default attributes...
> 
> Are we actually racing against userspace in a subsys initcall?

You can be, if you subsys is a module :)

thanks,

greg k-h

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

[Andrew Donnellan]

On 3/5/19 6:35 pm, Greg KH wrote:
>> Are we actually racing against userspace in a subsys initcall?
> 
> You can be, if you subsys is a module :)

For various reasons, we don't compile core system firmware interfaces 
into modules... that could be an interesting exercise. :D

-- 
Andrew Donnellan              OzLabs, ADL Canberra
ajd@linux.ibm.com             IBM Australia Limited

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

[Andrew Donnellan]

On 3/5/19 5:52 pm, 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>

mpe: ping?

> 
> ---
> 
> 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
> +};
>   
>   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);
>   	if (rc)
>   		pr_warn("Error %d creating OPAL symbols file\n", rc);
>   }
> 

-- 
Andrew Donnellan              OzLabs, ADL Canberra
ajd@linux.ibm.com             IBM Australia Limited

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

[Michael Ellerman]

Andrew Donnellan <ajd@linux.ibm.com> writes:
> On 3/5/19 5:52 pm, 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>
>
> mpe: ping?

Picked up for v5.4.

cheers

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

[Michael Ellerman]

On Fri, 2019-05-03 at 07:52:53 UTC, 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>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/e7de4f7b64c23e503a8c42af98d56f2a7462bd6d

cheers