Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

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

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Russell King (Oracle)]

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

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

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

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Mark Rutland]

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

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

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

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Russell King (Oracle)]

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

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

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

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Russell King (Oracle)]

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

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Mark Rutland]

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

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Kees Cook]

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