On Wed, Jun 16, 2021 at 11:32 AM Eric W. Biederman
<[email protected]> wrote:
>
> Prevent security holes by recording when all of the registers are
> available so generic code changes do not result in security holes
> on alpha.
Please no, not this way. ldl/stc is extremely expensive on some alpha cpus.
I really think thatTIF_ALLREGS_SAVED bit isn't worth it, except
perhaps for debugging.
And even for debugging, I think it would be both easier and cheaper to
just add a magic word to the entry stack instead.
Linus
Linus Torvalds <[email protected]> writes:
> On Wed, Jun 16, 2021 at 11:32 AM Eric W. Biederman
> <[email protected]> wrote:
>>
>> Prevent security holes by recording when all of the registers are
>> available so generic code changes do not result in security holes
>> on alpha.
>
> Please no, not this way. ldl/stc is extremely expensive on some alpha cpus.
>
> I really think thatTIF_ALLREGS_SAVED bit isn't worth it, except
> perhaps for debugging.
>
> And even for debugging, I think it would be both easier and cheaper to
> just add a magic word to the entry stack instead.
I think I can do something like that.
Looking at arch/alpha/asm/cache.h it looks like alpha had either 32byte
or 64bit cachelines. Which makes struct switch_stack a full 10 or 5
cachelines in size. So pushing something extra might hit an extra
cacheline.
However it looks like struct pt_regs is 16 bytes short of a full cache
line so struct switch_stack isn't going to be cacheline aligned. Adding
an extra 8 bytes of magic number will hopefully be in the noise.
If I can I would like to find something that is cheap enough that I can
always leave on. Mostly because there is little enough testing that a
bug that allows anyone to stomp the kernel stack has existed for 17 years
without being noticed.
If you want it to be a debug option only I can certainly make that
happen. I am still going "Eek! Arbitrary stack smash!" in my head.
Eric
On Wed, Jun 16, 2021 at 1:00 PM Linus Torvalds
<[email protected]> wrote:
>
> And even for debugging, I think it would be both easier and cheaper to
> just add a magic word to the entry stack instead.
IOW, just add a
unsigned long magic;
to "struct switch_stack", and then make the stack switch code push that value.
That would be cheap enough to be just unconditional, but you could
make it depend on a debug config option too, of course.
It helps if 'xyz' is some constant that is easyish to generate. It
might not be a constant - maybe it could be the address of that
'magic' field itself, so you'd just generate it with
stq $r,($r)
and it would be equally easy to just validate at lookup for that WARN_ON_ONCE():
WARN_ON_ONCE(switch_stack->magic != (unsigned long)&switch_stack->magic);
or whatever.
It's for debugging, not security. So it doesn't have to be some kind
of super-great magic number, just something easy to generate and check
(that isn't a common value like "0" that trivially exist on the stack
anyway).
Linus
Linus Torvalds <[email protected]> writes:
> On Wed, Jun 16, 2021 at 1:00 PM Linus Torvalds
> <[email protected]> wrote:
>>
>> And even for debugging, I think it would be both easier and cheaper to
>> just add a magic word to the entry stack instead.
>
> IOW, just add a
>
> unsigned long magic;
>
> to "struct switch_stack", and then make the stack switch code push that value.
>
> That would be cheap enough to be just unconditional, but you could
> make it depend on a debug config option too, of course.
>
> It helps if 'xyz' is some constant that is easyish to generate. It
> might not be a constant - maybe it could be the address of that
> 'magic' field itself, so you'd just generate it with
>
> stq $r,($r)
>
> and it would be equally easy to just validate at lookup for that WARN_ON_ONCE():
>
> WARN_ON_ONCE(switch_stack->magic != (unsigned long)&switch_stack->magic);
>
> or whatever.
>
> It's for debugging, not security. So it doesn't have to be some kind
> of super-great magic number, just something easy to generate and check
> (that isn't a common value like "0" that trivially exist on the stack
> anyway).
Fair enough.
I was thinking for a moment that do_sigreturn might have a problem with
that but restore_sigcontext makes it clear that struct switch_stack is
not exposed to userspace.
Do you know if struct switch_stack or pt_regs is ever exposeed to
usespace? They are both defined in arch/alpha/include/uapi/asm/ptrace.h
which makes me think userspace must see those definitions somewhere.
Eric
On Wed, Jun 16, 2021 at 03:57:19PM -0500, Eric W. Biederman wrote:
> Do you know if struct switch_stack or pt_regs is ever exposeed to
> usespace? They are both defined in arch/alpha/include/uapi/asm/ptrace.h
> which makes me think userspace must see those definitions somewhere.
They are exposed, but why mess with those in the first
place? thread_info->status is strictly thread-synchronous, so just
use it and be done with that...
On Wed, Jun 16, 2021 at 1:57 PM Eric W. Biederman <[email protected]> wrote:
>
> Do you know if struct switch_stack or pt_regs is ever exposeed to
> usespace? They are both defined in arch/alpha/include/uapi/asm/ptrace.h
> which makes me think userspace must see those definitions somewhere.
Yeah, that uapi location is a bit unfortunate. It means that user
space _could_ have seen it.
Which probably means that some user space uses it. Not for any kernel
interfaces (the alpha ptrace register offsets are actually sane, and
we have that "regoff[]" array to find them) - but I could see some odd
program having decided to use the kernel pt_regs and switch_stack
structures for their own reasons.
Annoying. Because we don't really expose it as-is in any way, afaik.
Only incidentally - and by mistake - in a uapi header file.
Maybe a flag in thread_info->status (or even a new 32-bit field
entirely in thread_info) is the way to go like Al says.
Linus