* Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set [not found] <0000000000004cf5c205faf1c7f3@google.com> @ 2024-03-05 11:27 ` Tetsuo Handa 2024-04-03 16:12 ` Russell King (Oracle) 0 siblings, 1 reply; 10+ messages in thread From: Tetsuo Handa @ 2024-03-05 11:27 UTC (permalink / raw) To: Linux ARM; +Cc: syzbot, linux-kernel, syzkaller-bugs Hello. syzbot is reporting kernel memory overwrite attempt at fpa_set(). I guessed that the amount to copy from/to should be sizeof(union fp_state) than sizeof(struct user_fp), for arch_ptrace(PTRACE_[SG]ETFPREGS) for arm is using offset == 0 and size == sizeof(union fp_state). But my guess did not solve the issue ( https://syzkaller.appspot.com/x/patch.diff?x=11e46dbc180000 ). On 2023/05/05 21:53, syzbot wrote: > Hello, > > syzbot found the following issue on: > > HEAD commit: 457391b03803 Linux 6.3 > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=105b8bb0280000 > kernel config: https://syzkaller.appspot.com/x/.config?x=385e197a58ca4afe > dashboard link: https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14 > compiler: arm-linux-gnueabi-gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 > userspace arch: arm #syz set subsystems: arm _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set 2024-03-05 11:27 ` [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set Tetsuo Handa @ 2024-04-03 16:12 ` Russell King (Oracle) 2024-04-05 14:28 ` Tetsuo Handa 2024-04-15 9:02 ` Mark Rutland 0 siblings, 2 replies; 10+ messages in thread From: Russell King (Oracle) @ 2024-04-03 16:12 UTC (permalink / raw) To: Tetsuo Handa; +Cc: Linux ARM, syzbot, linux-kernel, syzkaller-bugs On Tue, Mar 05, 2024 at 08:27:07PM +0900, Tetsuo Handa wrote: > Hello. > > syzbot is reporting kernel memory overwrite attempt at fpa_set(). > I guessed that the amount to copy from/to should be sizeof(union fp_state) > than sizeof(struct user_fp), for arch_ptrace(PTRACE_[SG]ETFPREGS) for arm > is using offset == 0 and size == sizeof(union fp_state). But my guess did not > solve the issue ( https://syzkaller.appspot.com/x/patch.diff?x=11e46dbc180000 ). This is silly. sizeof(struct user_fp) is: 8 * ( 1 bit for sign1, 15 bits unused => 2 bytes 1 bit for sign2, 14 bits unused, 1 bit for j => 2 bytes 31 bits for mantissa1 => 4 bytes 32 bits for mantissa0 => 4 bytes ) + 4 bytes for fpsr 4 bytes for fpcr 8 bytes for ftype 4 bytes for init_flag This totals 8 * 12 + 4 + 4 + 8 + 4 = 116 bytes or 29 32-bit quantities, or 29 "unsigned int"s. This is copied into union fp_state. This union is made up of one of several different formats depending on the FP being used. user_fp doesn't reflect this. However, one of these, struct fp_soft_struct, is specifically sized to ensure that user_fp is _smaller_. struct fp_soft_struct is 35 unsigned int's. This is 140 bytes. This is larger than sizeof(user_fp). Therefore, there is _no way_ for fpa_set() to overwrite anything outside of thread_info->fpstate, because sizeof(struct user_fp) is smaller than sizeof(thread->fpstate). Syzbot appears to be wrong in this instance. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set 2024-04-03 16:12 ` Russell King (Oracle) @ 2024-04-05 14:28 ` Tetsuo Handa 2024-04-15 9:02 ` Mark Rutland 1 sibling, 0 replies; 10+ messages in thread From: Tetsuo Handa @ 2024-04-05 14:28 UTC (permalink / raw) To: Russell King (Oracle), Kees Cook Cc: Linux ARM, syzbot, linux-kernel, syzkaller-bugs On 2024/04/04 1:12, Russell King (Oracle) wrote: > Therefore, there is _no way_ for fpa_set() to overwrite anything > outside of thread_info->fpstate, because sizeof(struct user_fp) > is smaller than sizeof(thread->fpstate). > > Syzbot appears to be wrong in this instance. > Thanks for clarification. I came to suspect that commit 08626a6056aa ("arm: Implement thread_struct whitelist for hardened usercopy") missed that ptrace(PTRACE_SETFPREGS) needs to declare a usercopy whitelist. It seems to me that https://syzkaller.appspot.com/text?tag=Patch&x=14c42099180000 can fix this problem, but I'm not sure whether this is safe/correct. Can you check? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set 2024-04-03 16:12 ` Russell King (Oracle) 2024-04-05 14:28 ` Tetsuo Handa @ 2024-04-15 9:02 ` Mark Rutland 2024-04-15 9:38 ` Tetsuo Handa 1 sibling, 1 reply; 10+ messages in thread From: Mark Rutland @ 2024-04-15 9:02 UTC (permalink / raw) To: Russell King (Oracle), Kees Cook Cc: Tetsuo Handa, Linux ARM, syzbot, linux-kernel, syzkaller-bugs On Wed, Apr 03, 2024 at 05:12:07PM +0100, Russell King (Oracle) wrote: > On Tue, Mar 05, 2024 at 08:27:07PM +0900, Tetsuo Handa wrote: > > Hello. > > > > syzbot is reporting kernel memory overwrite attempt at fpa_set(). > > I guessed that the amount to copy from/to should be sizeof(union fp_state) > > than sizeof(struct user_fp), for arch_ptrace(PTRACE_[SG]ETFPREGS) for arm > > is using offset == 0 and size == sizeof(union fp_state). But my guess did not > > solve the issue ( https://syzkaller.appspot.com/x/patch.diff?x=11e46dbc180000 ). > > This is silly. > > sizeof(struct user_fp) is: > > 8 * ( > 1 bit for sign1, 15 bits unused => 2 bytes > 1 bit for sign2, 14 bits unused, 1 bit for j => 2 bytes > 31 bits for mantissa1 => 4 bytes > 32 bits for mantissa0 => 4 bytes > ) + > 4 bytes for fpsr > 4 bytes for fpcr > 8 bytes for ftype > 4 bytes for init_flag > > This totals 8 * 12 + 4 + 4 + 8 + 4 = 116 bytes or 29 32-bit quantities, > or 29 "unsigned int"s. > > This is copied into union fp_state. This union is made up of one of > several different formats depending on the FP being used. user_fp > doesn't reflect this. However, one of these, struct fp_soft_struct, > is specifically sized to ensure that user_fp is _smaller_. > > struct fp_soft_struct is 35 unsigned int's. This is 140 bytes. This > is larger than sizeof(user_fp). > > Therefore, there is _no way_ for fpa_set() to overwrite anything > outside of thread_info->fpstate, because sizeof(struct user_fp) > is smaller than sizeof(thread->fpstate). > > Syzbot appears to be wrong in this instance. I believe the problem here is that HARDENED_USERCOPY tries to prevent any usercopy to/from task_struct except for fields that are explicitly whitelisted (which all need to be in one contiguous range). That was added in commit: 5905429ad85657c2 ("fork: Provide usercopy whitelisting for task_struct") However, architectures only have the option to provide arch_thread_struct_whitelist() to whitelist some fields in thread_struct, not thread_info where the fp_state lives. On arm arch_thread_struct_whitelist() whitelists precisely nothing: static inline void arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size) { *offset = *size = 0; } ... which was added in commit: 08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy") That commit says that all accesses are bounce-buffered and bypass the check, but AFAICT the fpa_set() code hasn't changed since then, so either that was wrong or the user_regset_copyin() code has changed. Kees, I believe you need to look at this. See the dashboard page at: https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14 Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set 2024-04-15 9:02 ` Mark Rutland @ 2024-04-15 9:38 ` Tetsuo Handa 2024-04-15 9:44 ` Russell King (Oracle) 0 siblings, 1 reply; 10+ messages in thread From: Tetsuo Handa @ 2024-04-15 9:38 UTC (permalink / raw) To: Mark Rutland, Russell King (Oracle), Kees Cook Cc: Linux ARM, syzbot, linux-kernel, syzkaller-bugs On 2024/04/15 18:02, Mark Rutland wrote: > 08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy") > > That commit says that all accesses are bounce-buffered and bypass the check, > but AFAICT the fpa_set() code hasn't changed since then, so either that was > wrong or the user_regset_copyin() code has changed. Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set 2024-04-15 9:38 ` Tetsuo Handa @ 2024-04-15 9:44 ` Russell King (Oracle) 2024-04-15 9:58 ` Tetsuo Handa 0 siblings, 1 reply; 10+ messages in thread From: Russell King (Oracle) @ 2024-04-15 9:44 UTC (permalink / raw) To: Tetsuo Handa Cc: Mark Rutland, Kees Cook, Linux ARM, syzbot, linux-kernel, syzkaller-bugs On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote: > On 2024/04/15 18:02, Mark Rutland wrote: > > 08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy") > > > > That commit says that all accesses are bounce-buffered and bypass the check, > > but AFAICT the fpa_set() code hasn't changed since then, so either that was > > wrong or the user_regset_copyin() code has changed. > > Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ? Have you visited that URL? It doesn't point to an email containing a patch, so sorry, I don't know what patch you're referring to. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set 2024-04-15 9:44 ` Russell King (Oracle) @ 2024-04-15 9:58 ` Tetsuo Handa 2024-04-15 10:27 ` Russell King (Oracle) 0 siblings, 1 reply; 10+ messages in thread From: Tetsuo Handa @ 2024-04-15 9:58 UTC (permalink / raw) To: Russell King (Oracle) Cc: Mark Rutland, Kees Cook, Linux ARM, syzbot, linux-kernel, syzkaller-bugs On 2024/04/15 18:44, Russell King (Oracle) wrote: > On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote: >> On 2024/04/15 18:02, Mark Rutland wrote: >>> 08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy") >>> >>> That commit says that all accesses are bounce-buffered and bypass the check, >>> but AFAICT the fpa_set() code hasn't changed since then, so either that was >>> wrong or the user_regset_copyin() code has changed. >> >> Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ? > > Have you visited that URL? It doesn't point to an email containing a > patch, so sorry, I don't know what patch you're referring to. > Containing a link to a diff. ;-) diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c index c421a899fc84..347611ae762f 100644 --- a/arch/arm/kernel/ptrace.c +++ b/arch/arm/kernel/ptrace.c @@ -583,10 +583,15 @@ static int fpa_set(struct task_struct *target, const void *kbuf, const void __user *ubuf) { struct thread_info *thread = task_thread_info(target); + const unsigned int pos0 = pos; + char buf[sizeof(struct user_fp)]; + int ret; - return user_regset_copyin(&pos, &count, &kbuf, &ubuf, - &thread->fpstate, - 0, sizeof(struct user_fp)); + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, + buf, 0, sizeof(struct user_fp)); + if (!ret) + memcpy(&thread->fpstate, buf, pos - pos0); + return ret; } #ifdef CONFIG_VFP _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set 2024-04-15 9:58 ` Tetsuo Handa @ 2024-04-15 10:27 ` Russell King (Oracle) 2024-04-15 11:43 ` Mark Rutland 0 siblings, 1 reply; 10+ messages in thread From: Russell King (Oracle) @ 2024-04-15 10:27 UTC (permalink / raw) To: Tetsuo Handa Cc: Mark Rutland, Kees Cook, Linux ARM, syzbot, linux-kernel, syzkaller-bugs On Mon, Apr 15, 2024 at 06:58:30PM +0900, Tetsuo Handa wrote: > On 2024/04/15 18:44, Russell King (Oracle) wrote: > > On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote: > >> On 2024/04/15 18:02, Mark Rutland wrote: > >>> 08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy") > >>> > >>> That commit says that all accesses are bounce-buffered and bypass the check, > >>> but AFAICT the fpa_set() code hasn't changed since then, so either that was > >>> wrong or the user_regset_copyin() code has changed. > >> > >> Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ? > > > > Have you visited that URL? It doesn't point to an email containing a > > patch, so sorry, I don't know what patch you're referring to. > > > > Containing a link to a diff. ;-) > > diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c > index c421a899fc84..347611ae762f 100644 > --- a/arch/arm/kernel/ptrace.c > +++ b/arch/arm/kernel/ptrace.c > @@ -583,10 +583,15 @@ static int fpa_set(struct task_struct *target, > const void *kbuf, const void __user *ubuf) > { > struct thread_info *thread = task_thread_info(target); > + const unsigned int pos0 = pos; > + char buf[sizeof(struct user_fp)]; > + int ret; > > - return user_regset_copyin(&pos, &count, &kbuf, &ubuf, > - &thread->fpstate, > - 0, sizeof(struct user_fp)); > + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, > + buf, 0, sizeof(struct user_fp)); > + if (!ret) > + memcpy(&thread->fpstate, buf, pos - pos0); > + return ret; > } > > #ifdef CONFIG_VFP No, not unless there is really no other option. It's hacking around the issue, creating two copy operations of the data (one onto the stack) rather than solving it properly - and I will not put up with that kind of mentality - it's a completely broken approach to open source software. If there is a problem, always fix it using the correct fix, never try to sticky-plaster around a problem. It seems there is a way for architectures to tell the code what is safe to write to, and it seems that a misunderstanding meant this wasn't implemented. So let's see whether it's possible to fix that first. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set 2024-04-15 10:27 ` Russell King (Oracle) @ 2024-04-15 11:43 ` Mark Rutland 2024-04-15 17:02 ` Kees Cook 0 siblings, 1 reply; 10+ messages in thread From: Mark Rutland @ 2024-04-15 11:43 UTC (permalink / raw) To: Russell King (Oracle) Cc: Tetsuo Handa, Kees Cook, Linux ARM, syzbot, linux-kernel, syzkaller-bugs On Mon, Apr 15, 2024 at 11:27:02AM +0100, Russell King (Oracle) wrote: > On Mon, Apr 15, 2024 at 06:58:30PM +0900, Tetsuo Handa wrote: > > On 2024/04/15 18:44, Russell King (Oracle) wrote: > > > On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote: > > >> On 2024/04/15 18:02, Mark Rutland wrote: > > >>> 08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy") > > >>> > > >>> That commit says that all accesses are bounce-buffered and bypass the check, > > >>> but AFAICT the fpa_set() code hasn't changed since then, so either that was > > >>> wrong or the user_regset_copyin() code has changed. > > >> > > >> Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ? > > > > > > Have you visited that URL? It doesn't point to an email containing a > > > patch, so sorry, I don't know what patch you're referring to. > > > > > > > Containing a link to a diff. ;-) > > > > diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c > > index c421a899fc84..347611ae762f 100644 > > --- a/arch/arm/kernel/ptrace.c > > +++ b/arch/arm/kernel/ptrace.c > > @@ -583,10 +583,15 @@ static int fpa_set(struct task_struct *target, > > const void *kbuf, const void __user *ubuf) > > { > > struct thread_info *thread = task_thread_info(target); > > + const unsigned int pos0 = pos; > > + char buf[sizeof(struct user_fp)]; > > + int ret; > > > > - return user_regset_copyin(&pos, &count, &kbuf, &ubuf, > > - &thread->fpstate, > > - 0, sizeof(struct user_fp)); > > + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, > > + buf, 0, sizeof(struct user_fp)); > > + if (!ret) > > + memcpy(&thread->fpstate, buf, pos - pos0); > > + return ret; > > } > > > > #ifdef CONFIG_VFP > > No, not unless there is really no other option. It's hacking around the > issue, creating two copy operations of the data (one onto the stack) > rather than solving it properly - and I will not put up with that kind > of mentality - it's a completely broken approach to open source > software. If there is a problem, always fix it using the correct fix, > never try to sticky-plaster around a problem. > > It seems there is a way for architectures to tell the code what is > safe to write to, and it seems that a misunderstanding meant this > wasn't implemented. So let's see whether it's possible to fix that > first. I completely agree. We'll have to wait for Kees to wake up, but IIUC one assumption here was that thread_info was particularly sensitive, and hence any state to be copied to/from userspace would live in thread_struct. Either we need to remove that assumption, or we need to move things so that we can use arch_thread_struct_whitelist(). Given that arm always selects THREAD_INFO_IN_TASK, I think it would be a fairly mechanical change to move fp_state (and vfp_state!) into thread_struct. That would mean that the TI_FPSTATE offset would grow, but assuming that still fits into an ADD immediate, we'd be ok, and then arch_thread_struct_whitelist() could be used to handle these structures. Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set 2024-04-15 11:43 ` Mark Rutland @ 2024-04-15 17:02 ` Kees Cook 0 siblings, 0 replies; 10+ messages in thread From: Kees Cook @ 2024-04-15 17:02 UTC (permalink / raw) To: Mark Rutland Cc: Russell King (Oracle), Tetsuo Handa, Linux ARM, syzbot, linux-kernel, syzkaller-bugs On Mon, Apr 15, 2024 at 12:43:59PM +0100, Mark Rutland wrote: > On Mon, Apr 15, 2024 at 11:27:02AM +0100, Russell King (Oracle) wrote: > > On Mon, Apr 15, 2024 at 06:58:30PM +0900, Tetsuo Handa wrote: > > > On 2024/04/15 18:44, Russell King (Oracle) wrote: > > > > On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote: > > > >> On 2024/04/15 18:02, Mark Rutland wrote: > > > >>> 08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy") > > > >>> > > > >>> That commit says that all accesses are bounce-buffered and bypass the check, > > > >>> but AFAICT the fpa_set() code hasn't changed since then, so either that was > > > >>> wrong or the user_regset_copyin() code has changed. > > > >> > > > >> Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ? > > > > > > > > Have you visited that URL? It doesn't point to an email containing a > > > > patch, so sorry, I don't know what patch you're referring to. > > > > > > > > > > Containing a link to a diff. ;-) > > > > > > diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c > > > index c421a899fc84..347611ae762f 100644 > > > --- a/arch/arm/kernel/ptrace.c > > > +++ b/arch/arm/kernel/ptrace.c > > > @@ -583,10 +583,15 @@ static int fpa_set(struct task_struct *target, > > > const void *kbuf, const void __user *ubuf) > > > { > > > struct thread_info *thread = task_thread_info(target); > > > + const unsigned int pos0 = pos; > > > + char buf[sizeof(struct user_fp)]; > > > + int ret; > > > > > > - return user_regset_copyin(&pos, &count, &kbuf, &ubuf, > > > - &thread->fpstate, > > > - 0, sizeof(struct user_fp)); > > > + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, > > > + buf, 0, sizeof(struct user_fp)); > > > + if (!ret) > > > + memcpy(&thread->fpstate, buf, pos - pos0); > > > + return ret; > > > } > > > > > > #ifdef CONFIG_VFP > > > > No, not unless there is really no other option. It's hacking around the > > issue, creating two copy operations of the data (one onto the stack) > > rather than solving it properly - and I will not put up with that kind > > of mentality - it's a completely broken approach to open source > > software. If there is a problem, always fix it using the correct fix, > > never try to sticky-plaster around a problem. > > > > It seems there is a way for architectures to tell the code what is > > safe to write to, and it seems that a misunderstanding meant this > > wasn't implemented. So let's see whether it's possible to fix that > > first. > > I completely agree. FWIW, the bound buffer approach is used in other places as well (especially for places that are not fastpath, like this case), as it creates an explicit bounds check (i.e. the buffer on the stack). But yes, if there is a way to specifically allow this during the kmem setup, let's do that. The timeline on this is interesting, it first showed up on syzbot about a year ago (2023/05/05 12:53) https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14 But hardened usercopy (and commit 08626a6056aad824) are from Jan 2018. No one noticed for 5 years? :P > We'll have to wait for Kees to wake up, but IIUC one assumption here was that > thread_info was particularly sensitive, and hence any state to be copied > to/from userspace would live in thread_struct. Either we need to remove that > assumption, or we need to move things so that we can use > arch_thread_struct_whitelist(). > > Given that arm always selects THREAD_INFO_IN_TASK, I think it would be a fairly > mechanical change to move fp_state (and vfp_state!) into thread_struct. That > would mean that the TI_FPSTATE offset would grow, but assuming that still fits > into an ADD immediate, we'd be ok, and then arch_thread_struct_whitelist() > could be used to handle these structures. Sure, or use a bounce buffer, since no one noticed this for 5 years it probably isn't heavy used[1]? But sure, I'm open to whatever -- the point is to not arbitrarily allow usercopy to touch the memory here. -Kees [1] We probably can't remove PTRACE_SETFPREGS, but yeah, it's really uncommon: https://codesearch.debian.net/search?q=ptrace%28PTRACE_SETFPREGS%2C&literal=1&perpkg=1 - systemtap test suite uses it - rr is x86 only - crui doesn't have a arm backend - python ptrace may use it - edb-debugger is x86 only - llvm mentions it in comments only -- Kees Cook _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-04-15 17:05 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <0000000000004cf5c205faf1c7f3@google.com>
2024-03-05 11:27 ` [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set Tetsuo Handa
2024-04-03 16:12 ` Russell King (Oracle)
2024-04-05 14:28 ` Tetsuo Handa
2024-04-15 9:02 ` Mark Rutland
2024-04-15 9:38 ` Tetsuo Handa
2024-04-15 9:44 ` Russell King (Oracle)
2024-04-15 9:58 ` Tetsuo Handa
2024-04-15 10:27 ` Russell King (Oracle)
2024-04-15 11:43 ` Mark Rutland
2024-04-15 17:02 ` Kees Cook
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).