* [PATCH] [RFC]arm64:Mark __stack_chk_guard as __ro_after_init @ 2021-09-14 9:44 Dan Li 2021-09-14 9:58 ` Russell King (Oracle) ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Dan Li @ 2021-09-14 9:44 UTC (permalink / raw) To: catalin.marinas, will; +Cc: linux-arm-kernel, linux-kernel, Dan Li __stack_chk_guard is setup once while init stage and never changed after that. Although the modification of this variable at runtime will usually cause the kernel to crash (so dose the attacker), it should be marked as _ro_after_init, and it should not affect performance if it is placed in the ro_after_init section. This should also be the case on the ARM platform, or am I missing something? Signed-off-by: Dan Li <ashimida@linux.alibaba.com> --- arch/arm64/kernel/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index c8989b9..c858b85 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -60,7 +60,7 @@ #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK) #include <linux/stackprotector.h> -unsigned long __stack_chk_guard __read_mostly; +unsigned long __stack_chk_guard __ro_after_init; EXPORT_SYMBOL(__stack_chk_guard); #endif -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] [RFC]arm64:Mark __stack_chk_guard as __ro_after_init 2021-09-14 9:44 [PATCH] [RFC]arm64:Mark __stack_chk_guard as __ro_after_init Dan Li @ 2021-09-14 9:58 ` Russell King (Oracle) 2021-09-14 10:17 ` Mark Rutland 2021-09-16 17:08 ` Catalin Marinas 2 siblings, 0 replies; 7+ messages in thread From: Russell King (Oracle) @ 2021-09-14 9:58 UTC (permalink / raw) To: Dan Li; +Cc: catalin.marinas, will, linux-arm-kernel, linux-kernel On Tue, Sep 14, 2021 at 05:44:02PM +0800, Dan Li wrote: > __stack_chk_guard is setup once while init stage and never changed > after that. > > Although the modification of this variable at runtime will usually > cause the kernel to crash (so dose the attacker), it should be marked > as _ro_after_init, and it should not affect performance if it is > placed in the ro_after_init section. > > This should also be the case on the ARM platform, or am I missing > something? I don't see why it can't be - we only write to it in boot_init_stack_canary(), same as ARM64. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] [RFC]arm64:Mark __stack_chk_guard as __ro_after_init 2021-09-14 9:44 [PATCH] [RFC]arm64:Mark __stack_chk_guard as __ro_after_init Dan Li 2021-09-14 9:58 ` Russell King (Oracle) @ 2021-09-14 10:17 ` Mark Rutland 2021-09-15 1:57 ` ashimida 2021-09-16 17:08 ` Catalin Marinas 2 siblings, 1 reply; 7+ messages in thread From: Mark Rutland @ 2021-09-14 10:17 UTC (permalink / raw) To: Dan Li Cc: catalin.marinas, will, linux-arm-kernel, linux-kernel, Laura Abbott On Tue, Sep 14, 2021 at 05:44:02PM +0800, Dan Li wrote: > __stack_chk_guard is setup once while init stage and never changed > after that. > > Although the modification of this variable at runtime will usually > cause the kernel to crash (so dose the attacker), it should be marked > as _ro_after_init, and it should not affect performance if it is > placed in the ro_after_init section. > > This should also be the case on the ARM platform, or am I missing > something? > > Signed-off-by: Dan Li <ashimida@linux.alibaba.com> FWIW, this makes sense to me: Acked-by: Mark Rutland <mark.rutland@arm.com> Looking at the history, this was added to arm64 in commit: c0c264ae5112d1cd ("arm64: Add CONFIG_CC_STACKPROTECTOR") ... whereas __ro_after_init was introduced around 2 years later in commit: c74ba8b3480da6dd ("arch: Introduce post-init read-only memory") ... so we weren't deliberately avoiding __ro_after_init, and there are probably a significant number of other variables we could apply it to. Mark. > --- > arch/arm64/kernel/process.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index c8989b9..c858b85 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -60,7 +60,7 @@ > > #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK) > #include <linux/stackprotector.h> > -unsigned long __stack_chk_guard __read_mostly; > +unsigned long __stack_chk_guard __ro_after_init; > EXPORT_SYMBOL(__stack_chk_guard); > #endif > > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] [RFC]arm64:Mark __stack_chk_guard as __ro_after_init 2021-09-14 10:17 ` Mark Rutland @ 2021-09-15 1:57 ` ashimida 2021-09-15 9:19 ` Mark Rutland 0 siblings, 1 reply; 7+ messages in thread From: ashimida @ 2021-09-15 1:57 UTC (permalink / raw) To: Mark Rutland Cc: catalin.marinas, will, linux-arm-kernel, linux-kernel, Laura Abbott Hi King, Rutland: Thanks for the reply and let me understand the reason here. Then may I first submit a patch to modify the attributes of __stack_chk_guard of the arm/aarch64 platform? On 9/14/21 6:17 PM, Mark Rutland wrote: > On Tue, Sep 14, 2021 at 05:44:02PM +0800, Dan Li wrote: >> __stack_chk_guard is setup once while init stage and never changed >> after that. >> >> Although the modification of this variable at runtime will usually >> cause the kernel to crash (so dose the attacker), it should be marked >> as _ro_after_init, and it should not affect performance if it is >> placed in the ro_after_init section. >> >> This should also be the case on the ARM platform, or am I missing >> something? >> >> Signed-off-by: Dan Li <ashimida@linux.alibaba.com> > > FWIW, this makes sense to me: > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > Looking at the history, this was added to arm64 in commit: > > c0c264ae5112d1cd ("arm64: Add CONFIG_CC_STACKPROTECTOR") > > ... whereas __ro_after_init was introduced around 2 years later in > commit: > > c74ba8b3480da6dd ("arch: Introduce post-init read-only memory") > > ... so we weren't deliberately avoiding __ro_after_init, and there are > probably a significant number of other variables we could apply it to. > > Mark. > >> --- >> arch/arm64/kernel/process.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >> index c8989b9..c858b85 100644 >> --- a/arch/arm64/kernel/process.c >> +++ b/arch/arm64/kernel/process.c >> @@ -60,7 +60,7 @@ >> >> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK) >> #include <linux/stackprotector.h> >> -unsigned long __stack_chk_guard __read_mostly; >> +unsigned long __stack_chk_guard __ro_after_init; >> EXPORT_SYMBOL(__stack_chk_guard); >> #endif >> >> -- >> 2.7.4 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] [RFC]arm64:Mark __stack_chk_guard as __ro_after_init 2021-09-15 1:57 ` ashimida @ 2021-09-15 9:19 ` Mark Rutland 2021-09-15 9:35 ` Dan Li 0 siblings, 1 reply; 7+ messages in thread From: Mark Rutland @ 2021-09-15 9:19 UTC (permalink / raw) To: ashimida Cc: catalin.marinas, will, linux-arm-kernel, linux-kernel, Laura Abbott On Wed, Sep 15, 2021 at 09:57:14AM +0800, ashimida wrote: > Hi King, Rutland: > > Thanks for the reply and let me understand the reason here. > > Then may I first submit a patch to modify the attributes of > __stack_chk_guard of the arm/aarch64 platform? This patch looks fine as-is (hence the Acked-by). Doing the same for arch/arm makes sense, but that should be a separate patch. I was suggesting that in future we should probably do the same in more places, not that you need to do so now. Thanks, Mark. > > On 9/14/21 6:17 PM, Mark Rutland wrote: > > On Tue, Sep 14, 2021 at 05:44:02PM +0800, Dan Li wrote: > > > __stack_chk_guard is setup once while init stage and never changed > > > after that. > > > > > > Although the modification of this variable at runtime will usually > > > cause the kernel to crash (so dose the attacker), it should be marked > > > as _ro_after_init, and it should not affect performance if it is > > > placed in the ro_after_init section. > > > > > > This should also be the case on the ARM platform, or am I missing > > > something? > > > > > > Signed-off-by: Dan Li <ashimida@linux.alibaba.com> > > > > FWIW, this makes sense to me: > > > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > > > Looking at the history, this was added to arm64 in commit: > > > > c0c264ae5112d1cd ("arm64: Add CONFIG_CC_STACKPROTECTOR") > > > > ... whereas __ro_after_init was introduced around 2 years later in > > commit: > > > > c74ba8b3480da6dd ("arch: Introduce post-init read-only memory") > > > > ... so we weren't deliberately avoiding __ro_after_init, and there are > > probably a significant number of other variables we could apply it to. > > > > Mark. > > > > > --- > > > arch/arm64/kernel/process.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > > index c8989b9..c858b85 100644 > > > --- a/arch/arm64/kernel/process.c > > > +++ b/arch/arm64/kernel/process.c > > > @@ -60,7 +60,7 @@ > > > #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK) > > > #include <linux/stackprotector.h> > > > -unsigned long __stack_chk_guard __read_mostly; > > > +unsigned long __stack_chk_guard __ro_after_init; > > > EXPORT_SYMBOL(__stack_chk_guard); > > > #endif > > > -- > > > 2.7.4 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] [RFC]arm64:Mark __stack_chk_guard as __ro_after_init 2021-09-15 9:19 ` Mark Rutland @ 2021-09-15 9:35 ` Dan Li 0 siblings, 0 replies; 7+ messages in thread From: Dan Li @ 2021-09-15 9:35 UTC (permalink / raw) To: Mark Rutland Cc: catalin.marinas, will, linux-arm-kernel, linux-kernel, Laura Abbott Fine, I got it, thanks for reply. On 9/15/21 5:19 PM, Mark Rutland wrote: > On Wed, Sep 15, 2021 at 09:57:14AM +0800, ashimida wrote: >> Hi King, Rutland: >> >> Thanks for the reply and let me understand the reason here. >> >> Then may I first submit a patch to modify the attributes of >> __stack_chk_guard of the arm/aarch64 platform? > > This patch looks fine as-is (hence the Acked-by). Doing the same for > arch/arm makes sense, but that should be a separate patch. > > I was suggesting that in future we should probably do the same in more > places, not that you need to do so now. > > Thanks, > Mark. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC]arm64:Mark __stack_chk_guard as __ro_after_init 2021-09-14 9:44 [PATCH] [RFC]arm64:Mark __stack_chk_guard as __ro_after_init Dan Li 2021-09-14 9:58 ` Russell King (Oracle) 2021-09-14 10:17 ` Mark Rutland @ 2021-09-16 17:08 ` Catalin Marinas 2 siblings, 0 replies; 7+ messages in thread From: Catalin Marinas @ 2021-09-16 17:08 UTC (permalink / raw) To: will, Dan Li; +Cc: linux-arm-kernel, linux-kernel On Tue, 14 Sep 2021 17:44:02 +0800, Dan Li wrote: > __stack_chk_guard is setup once while init stage and never changed > after that. > > Although the modification of this variable at runtime will usually > cause the kernel to crash (so dose the attacker), it should be marked > as _ro_after_init, and it should not affect performance if it is > placed in the ro_after_init section. > > [...] Applied to arm64 (for-next/fixes), thanks! [1/1] arm64:Mark __stack_chk_guard as __ro_after_init https://git.kernel.org/arm64/c/9fcb2e93f41c -- Catalin ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-09-16 17:52 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-14 9:44 [PATCH] [RFC]arm64:Mark __stack_chk_guard as __ro_after_init Dan Li 2021-09-14 9:58 ` Russell King (Oracle) 2021-09-14 10:17 ` Mark Rutland 2021-09-15 1:57 ` ashimida 2021-09-15 9:19 ` Mark Rutland 2021-09-15 9:35 ` Dan Li 2021-09-16 17:08 ` Catalin Marinas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).