| Message ID | 20131107175010.GI13674@arm.com |
|---|---|
| State | New |
| Headers | show |
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.
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 ;).
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?
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.
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.
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