Re: [PATCH v3 3/3] MIPS: set stack/data protection as non-executable

From: David Daney
Date: Fri Dec 05 2014 - 14:06:52 EST


On 12/05/2014 10:51 AM, Kees Cook wrote:
On Fri, Dec 5, 2014 at 9:28 AM, David Daney <ddaney.cavm@xxxxxxxxx> wrote:
On 12/02/2014 05:58 PM, Leonid Yegoshin wrote:

This is a last step of 3 patches which shift FPU emulation out of
stack into protected area. So, it disables a default executable stack.

Additionally, it sets a default data area non-executable protection.

Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@xxxxxxxxxx>


NAK!

Some programs require an executable stack, this patch will break them.

Have you tested this?

Do you require empirical evidence that the patch is incorrect, or is it enough to just to trust me when I say that it is incorrect? Typically the burden of proof is with those proposing the patches.


You can only select a non-executable stack in response to PT_GNU_STACK
program headers in the ELF file of the executable program.

This is already handled by fs/binfmt_elf.c. It does the parsing of the
PT_GNU_STACK needs, and sets up the stack flags appropriately. All the
change to VM_DATA_DEFAULT_FLAGS does is make sure that EXSTACK_DEFAULT
now means no VM_EXEC by default. If PT_GNU_STACK requires it, it gets
added back in.


The problem is not with "modern" executables that are properly annotated with PT_GNU_STACK.

My objection is to the intentional breaking of old executables that have no PT_GNU_STACK annotation, but require an executable stack. Since we usually try not to break userspace, we cannot merge a patch like this one.

David Daney.


-Kees


David Daney



---
arch/mips/include/asm/page.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/page.h b/arch/mips/include/asm/page.h
index 3be81803595d..d49ba81cb4ed 100644
--- a/arch/mips/include/asm/page.h
+++ b/arch/mips/include/asm/page.h
@@ -230,7 +230,7 @@ extern int __virt_addr_valid(const volatile void
*kaddr);
#define virt_addr_valid(kaddr)
\
__virt_addr_valid((const volatile void *) (kaddr))

-#define VM_DATA_DEFAULT_FLAGS (VM_READ | VM_WRITE | VM_EXEC | \
+#define VM_DATA_DEFAULT_FLAGS (VM_READ | VM_WRITE | \
VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC)

#define UNCAC_ADDR(addr) ((addr) - PAGE_OFFSET + UNCAC_BASE)









--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/