Re: [clang] stack protector and f1f029c7bf

[H. Peter Anvin]

On 05/24/18 13:26, Nick Desaulniers wrote:
> 
> Considering the bigger picture is why we have this thread going.  You have
> much more context about the kernel than I or the llvm maintainers do.  We
> can't consider the bigger picture until we ask.
> 

And this is correct. However, *every* LLVM request we get seem to be of
the form "if you apply this patch it will patch over one particular
instance of a problem with the kernel-LLVM."

There are two problems with that:

1. It focuses narrowly on an issue, which might (and more than a handful
   of times have) mask much more fundamental, serious bugs, in both the
   kernel and LLVM.

2. It makes it sound like an LLVM problem that wants to be hacked in the
   kernel. More than a handful of times that is exactly what they have
   been.

Look how many fundamental issues we have uncovered from this simple
issue, which would have gotten completely lost had we just applied this
patch.

>> Issue 1: Fundamentally,
> 
> Fundamentally, the Linux kernel should not rely on GCC's heuristics for
> adding or not adding a stack protector to functions with custom calling
> conventions that are not marked in any way to let the compiler know.

This is true, although it is actually NOT the fundamental issue (see
below) but you also deleted the exact LLVM problem that I pointed out:

COMPILER AR: "=rm" should NEVER generate worse code than "=r". That is
unequivocally a compiler bug.

The other problem here is that omitting the stack canary is really not
the right thing to do, either. The right thing to do is to add a
mechanism for the compiler to know to reserve some number of stack
slots. Ideally this should work even if a function includes an inline
function or macro that contains the actual inline assembly.

COMPILER AR: Add a mechanism for the compiler to know that inline
assembly needs a certain amount of slack stack space. In the ideal
universe, this should ALSO mean the compiler offsets all "m" references
that contain references to the stack pointer.

KERNEL AR: Add those annotations to all cases of inline assembly that
modifies the stack pointer.

>> You are claiming it doesn't buy us anything, but you are only looking at
> the paravirt case which is kind of "special" (in the short bus kind of way),
> 
> That's fair.  Is another possible solution to have paravirt maybe not use
> native_save_fl() then, but its own non-static-inline-without-m-constraint
> implementation?

KERNEL AR: change native_save_fl() to an extern inline with an assembly
out-of-line implementation, to satisfy the paravirt requirement that no
GPRs other than %rax are clobbered.

>> Issue 3: Let's face it, reading and writing the flags should be builtins,
> exactly because it has to do stack operations, which really means the
> compiler should be involved.
> 
> I'm happy to propose that as a feature request to llvm+gcc.

Being discussed elsewhere, of course.

	-hpa