[PATCH trivial] arm64: constify hwcap_str

[PATCH trivial] arm64: constify hwcap_str

[Ard Biesheuvel]

Turn hwcap_str from a 'writable array of pointers to const char[]'
to a multidimensional const char[][].

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

This is something I noticed when looking at Steve Capper's hwcaps patch: really
no point in having a writable array of 8-byte pointers in .data keeping track of
these hwcap strings.
 
 arch/arm64/kernel/setup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 055cfb8..b6aa6b4 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -301,10 +301,10 @@ static int __init topology_init(void)
 }
 subsys_initcall(topology_init);
 
-static const char *hwcap_str[] = {
+static const char hwcap_str[][8] = {
 	"fp",
 	"asimd",
-	NULL
+	""
 };
 
 static int c_show(struct seq_file *m, void *v)
@@ -328,7 +328,7 @@ static int c_show(struct seq_file *m, void *v)
 	/* dump out the processor features */
 	seq_puts(m, "Features\t: ");
 
-	for (i = 0; hwcap_str[i]; i++)
+	for (i = 0; hwcap_str[i][0]; i++)
 		if (elf_hwcap & (1 << i))
 			seq_printf(m, "%s ", hwcap_str[i]);
 
-- 
1.8.3.2

[PATCH trivial] arm64: constify hwcap_str

[Catalin Marinas]

On Wed, Nov 06, 2013 at 04:32:47PM +0000, Ard Biesheuvel wrote:
> Turn hwcap_str from a 'writable array of pointers to const char[]'
> to a multidimensional const char[][].
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> This is something I noticed when looking at Steve Capper's hwcaps patch: really
> no point in having a writable array of 8-byte pointers in .data keeping track of
> these hwcap strings.

I don't see a problem (few bytes wasted). If you want to make it
read-only, this would do:

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 9cf30f49610d..84a770fd7003 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -308,7 +308,7 @@ static int __init topology_init(void)
 }
 subsys_initcall(topology_init);
 
-static const char *hwcap_str[] = {
+static const char *const hwcap_str[] = {
 	"fp",
 	"asimd",
 	NULL

-- 
Catalin

[PATCH trivial] arm64: constify hwcap_str

[Ard Biesheuvel]

On 7 November 2013 18:50, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Wed, Nov 06, 2013 at 04:32:47PM +0000, Ard Biesheuvel wrote:
>> Turn hwcap_str from a 'writable array of pointers to const char[]'
>> to a multidimensional const char[][].
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>
>> This is something I noticed when looking at Steve Capper's hwcaps patch: really
>> no point in having a writable array of 8-byte pointers in .data keeping track of
>> these hwcap strings.
>
> I don't see a problem (few bytes wasted). If you want to make it
> read-only, this would do:
>
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 9cf30f49610d..84a770fd7003 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -308,7 +308,7 @@ static int __init topology_init(void)
>  }
>  subsys_initcall(topology_init);
>
> -static const char *hwcap_str[] = {
> +static const char *const hwcap_str[] = {
>         "fp",
>         "asimd",
>         NULL
>

I agree that whether you want to put up with the additional layer of
indirection, additional relocations etc is a matter of taste, but
having writable pointers around that are easily dereferenced directly
by unprivileged users is a bad idea in any case, so if this is your
preferred solution, it's fine by me.

Regards,
Ard.

[PATCH trivial] arm64: constify hwcap_str

[Catalin Marinas]

On Thu, Nov 07, 2013 at 06:53:06PM +0000, Ard Biesheuvel wrote:
> On 7 November 2013 18:50, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Wed, Nov 06, 2013 at 04:32:47PM +0000, Ard Biesheuvel wrote:
> >> Turn hwcap_str from a 'writable array of pointers to const char[]'
> >> to a multidimensional const char[][].
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>
> >> This is something I noticed when looking at Steve Capper's hwcaps patch: really
> >> no point in having a writable array of 8-byte pointers in .data keeping track of
> >> these hwcap strings.
> >
> > I don't see a problem (few bytes wasted). If you want to make it
> > read-only, this would do:
> >
> > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> > index 9cf30f49610d..84a770fd7003 100644
> > --- a/arch/arm64/kernel/setup.c
> > +++ b/arch/arm64/kernel/setup.c
> > @@ -308,7 +308,7 @@ static int __init topology_init(void)
> >  }
> >  subsys_initcall(topology_init);
> >
> > -static const char *hwcap_str[] = {
> > +static const char *const hwcap_str[] = {
> >         "fp",
> >         "asimd",
> >         NULL
> 
> I agree that whether you want to put up with the additional layer of
> indirection, additional relocations etc is a matter of taste, but
> having writable pointers around that are easily dereferenced directly
> by unprivileged users is a bad idea in any case,

"unprivileged users"?!

> so if this is your preferred solution, it's fine by me.

My preferred solution is to leave it as it is ;).

-- 
Catalin

[PATCH trivial] arm64: constify hwcap_str

[Ard Biesheuvel]

On 8 November 2013 15:38, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Thu, Nov 07, 2013 at 06:53:06PM +0000, Ard Biesheuvel wrote:
[...]
>> I agree that whether you want to put up with the additional layer of
>> indirection, additional relocations etc is a matter of taste, but
>> having writable pointers around that are easily dereferenced directly
>> by unprivileged users is a bad idea in any case,
>
> "unprivileged users"?!
>

Well, I know this may seem far fetched, but /proc/cpuinfo lacks any
kind of access control because it is deemed harmless, while, as an
unprivileged user, having some pointers in .data that you can clobber
(through some other vulnerability) without breaking anything, and can
conveniently dereference at your leisure by cat'ing /proc/cpuinfo, may
well be something that could potentially be used in the wrong way.

>> so if this is your preferred solution, it's fine by me.
>
> My preferred solution is to leave it as it is ;).
>

It's a trivial fix, so why not apply it?

-- 
Ard.

[PATCH trivial] arm64: constify hwcap_str

[Catalin Marinas]

On Fri, Nov 08, 2013 at 05:14:16PM +0000, Ard Biesheuvel wrote:
> On 8 November 2013 15:38, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Thu, Nov 07, 2013 at 06:53:06PM +0000, Ard Biesheuvel wrote:
> [...]
> >> I agree that whether you want to put up with the additional layer of
> >> indirection, additional relocations etc is a matter of taste, but
> >> having writable pointers around that are easily dereferenced directly
> >> by unprivileged users is a bad idea in any case,
> >
> > "unprivileged users"?!
> 
> Well, I know this may seem far fetched, but /proc/cpuinfo lacks any
> kind of access control because it is deemed harmless, while, as an
> unprivileged user, having some pointers in .data that you can clobber
> (through some other vulnerability) without breaking anything, and can
> conveniently dereference at your leisure by cat'ing /proc/cpuinfo, may
> well be something that could potentially be used in the wrong way.

That's far fetched indeed ;). I'm pretty sure once you can write the
.data section there are far better way to hack the kernel.

> >> so if this is your preferred solution, it's fine by me.
> >
> > My preferred solution is to leave it as it is ;).
> 
> It's a trivial fix, so why not apply it?

So far my toolchain already places it in .rodata since there is no write
to these pointers.

-- 
Catalin

[PATCH trivial] arm64: constify hwcap_str

[Ard Biesheuvel]

On 8 November 2013 18:19, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Nov 08, 2013 at 05:14:16PM +0000, Ard Biesheuvel wrote:
>> On 8 November 2013 15:38, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> > On Thu, Nov 07, 2013 at 06:53:06PM +0000, Ard Biesheuvel wrote:
>> It's a trivial fix, so why not apply it?
>
> So far my toolchain already places it in .rodata since there is no write
> to these pointers.
>

So the fix is even more trivial than I thought :-)

Cheers,
Ard.