Re: [kernel-hardening] Re: [PATCH v9 1/4] syscalls: Verify address limit before returning to user-mode

From: Andy Lutomirski
Date: Fri May 12 2017 - 02:14:24 EST


[resending because kernel.org seems to have mangled my SMTP
credentials. I wonder if this is a common problem.]

On Thu, May 11, 2017 at 4:44 PM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, May 11, 2017 at 4:17 PM, Thomas Garnier <thgarnie@xxxxxxxxxx> wrote:
>>
>> Ingo: Do you want the change as-is? Would you like it to be optional?
>> What do you think?
>
> I'm not ingo, but I don't like that patch. It's in the wrong place -
> that system call return code is too timing-critical to add address
> limit checks.
>
> Now what I think you *could* do is:
>
> - make "set_fs()" actually set a work flag in the current thread flags
>
> - do the test in the slow-path (syscall_return_slowpath).
>
> Yes, yes, that ends up being architecture-specific, but it's fairly simple.
>
> And it only slows down the system calls that actually use "set_fs()".
> Sure, it will slow those down a fair amount, but they are hopefully a
> small subset of all cases.
>
> How does that sound to people? Thats' where we currently do that
>
> if (IS_ENABLED(CONFIG_PROVE_LOCKING) &&
> WARN(irqs_disabled(), "syscall %ld left IRQs disabled",
> regs->orig_ax))
> local_irq_enable();
>
> check too, which is a fairly similar issue.
>

I like this. It wouldn't help the problem that I suspect is a major
part of the motivation for this patch: a stack overflow could
overwrite addr_limit. But we fixed that for real already.

Slightly off-topic: I would *love* to see syscall_return_slowpath() or
similar moved or at least mostly moved into generic code. Aside from
the fact that it used to be written in asm, there's nothing
fundamentally arch-specific about it.

>
> And it only slows down the system calls that actually use "set_fs()".
> Sure, it will slow those down a fair amount, but they are hopefully a
> small subset of all cases.

It won't even slow them down that much. The slow path is reasonably
fast these days.