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); }
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
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?
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
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
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 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
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
linux-stable-mirror@lists.linaro.org
-
Andrew Donnellan -
Greg KH -
Michael Ellerman -
Michael Ellerman