public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* commit 81106b7e0b13 can break asm_int80_emulation on x86_64
@ 2024-08-08  1:57 Bert Karwatzki
  0 siblings, 0 replies; 5+ messages in thread
From: Bert Karwatzki @ 2024-08-08  1:57 UTC (permalink / raw)
  To: mingo
  Cc: Bert Karwatzki, akpm, Oleg Nesterov, Andy Lutomirski,
	Borislav Petkov, Fenghua Yu, H . Peter Anvin, Linus Torvalds,
	Dave Hansen, Thomas Gleixner, Uros Bizjak, linux-kernel, peterz

Since linux-next-20240730 and in versions next-202408{05,06} the  error below
can appear when trying to start a game using wine. The error does not always
appear but after some failed attempts at bisecting this I found a way to
reliably trigger this by first creating high system load (by compiling a kernel
with make -j 16) and then trying to start the game until the error occurs. If the
error does not occur during the time it take to compile the kernel (about ~6.5min)
I declared the commit as good. Usually the bad commit did not take more than
three attempts of starting wine to trigger the error.
With this I bisected the problem to commit 81106b7e0b13.

To revert this commit in next-20240806 the following commits have to be reverted:
(HEAD -> wine_usercopy_bug) Revert "x86/fpu: Make task_struct::thread constant size" 
Revert "x86/fpu: Remove the thread::fpu pointer"
Revert "x86/fpu: Remove init_task FPU state dependencies, add debugging warning for PF_KTHREAD tasks"
Revert "x86/CPU/AMD: Always inline amd_clear_divider()"

After reverting these commit the next-20240806 seems to be free from the error.
The machine used is an Ryzen 5800H (Msi Alpha15 Laptop) running a fully updated
(as of 20240807) debian sid.

Bert Karwatzki

Error message:
[T34926] usercopy: Kernel memory overwrite attempt detected to SLUB object 'task_struct' (offset 3072, size 160)!
[T34926] ------------[ cut here ]------------
[T34926] kernel BUG at mm/usercopy.c:102!
[T34926] Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
[T34926] CPU: 12 UID: 1000 PID: 34926 Comm: start.exe Not tainted 6.11.0-rc2-next-20240806-master-dirty #178
[T34926] Hardware name: Micro-Star International Co., Ltd. Alpha 15 B5EEK/MS-158L, BIOS E158LAMS.107 11/10/2021
[T34926] RIP: 0010:usercopy_abort+0x73/0x75
[T34926] Code: e0 f2 06 86 eb 0e 48 c7 c2 81 6b 08 86 48 c7 c7 e9 f2 06 86 56 48 89 fe 48 c7 c7 d8 47 0c 86 51 48 89 c1 41 52 e8 6d 70 ff ff <0f> 0b 48 89 d9 49 89 e8 44 89 e2 31 f6 48 81 e9 00 00 20 85 48 c7
[T34926] RSP: 0000:ffffa58a47e73cd0 EFLAGS: 00010246
[T34926] RAX: 0000000000000068 RBX: ffff8e1eec1a4a00 RCX: 0000000000000000
[T34926] RDX: 0000000000000000 RSI: ffff8e2cae9177c0 RDI: ffff8e2cae9177c0
[T34926] RBP: 00000000000000a0 R08: 0000000000000000 R09: ffffa58a47e73b78
[T34926] R10: ffffffff8627fe88 R11: 0000000000000003 R12: 0000000000000000
[T34926] R13: ffff8e1eec1a4aa0 R14: ffff8e1eec1a4a00 R15: 0000000000000001
[T34926] FS:  000000003ffe2000(006b) GS:ffff8e2cae900000(0063) knlGS:00000000f7ed20c0
[T34926] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
[T34926] CR2: 000000003ffed324 CR3: 00000002a58da000 CR4: 0000000000750ef0
[T34926] PKRU: 55555554
[T34926] Call Trace:
[T34926]  <TASK>
[T34926]  ? __die+0x51/0x92
[T34926]  ? die+0x29/0x50
[T34926]  ? do_trap+0x105/0x110
[T34926]  ? do_error_trap+0x60/0x80
[T34926]  ? usercopy_abort+0x73/0x75
[T34926]  ? exc_invalid_op+0x4d/0x70
[T34926]  ? usercopy_abort+0x73/0x75
[T34926]  ? asm_exc_invalid_op+0x1a/0x20
[T34926]  ? usercopy_abort+0x73/0x75
[T34926]  ? __check_heap_object+0x7d/0xa0
[T34926]  ? __check_object_size+0x1f9/0x210
[T34926]  ? copy_from_buffer+0x40/0x60
[T34926]  ? copy_uabi_to_xstate+0xe1/0x1e0
[T34926]  ? __fpu_restore_sig+0x403/0x480
[T34926]  ? fpu__restore_sig+0x4c/0x90
[T34926]  ? ia32_restore_sigcontext+0x129/0x170
[T34926]  ? __do_compat_sys_rt_sigreturn+0x68/0xc0
[T34926]  ? do_int80_emulation+0x88/0x140
[T34926]  ? asm_int80_emulation+0x1a/0x20
[T34926]  </TASK>
[T34926] Modules linked in: ccm snd_seq_dummy snd_hrtimer snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device rfcomm bnep nls_ascii nls_cp437 vfat fat snd_ctl_led snd_hda_codec_realtek snd_hda_codec_generic snd_hda_scodec_component snd_hda_codec_hdmi btusb btrtl btintel snd_hda_intel btbcm btmtk snd_intel_dspcfg snd_acp3x_pdm_dma snd_soc_dmic snd_acp3x_rn uvcvideo bluetooth snd_hda_codec videobuf2_vmalloc snd_soc_core uvc snd_hwdep videobuf2_memops videobuf2_v4l2 snd_hda_core snd_pcm_oss videodev snd_mixer_oss snd_pcm snd_rn_pci_acp3x videobuf2_common snd_acp_config msi_wmi snd_soc_acpi ecdh_generic amd_atl ecc mc sparse_keymap edac_mce_amd wmi_bmof snd_timer snd ccp snd_pci_acp3x k10temp soundcore battery ac joydev button hid_sensor_accel_3d hid_sensor_prox hid_sensor_als hid_sensor_magn_3d hid_sensor_gyro_3d hid_sensor_trigger industrialio_triggered_buffer kfifo_buf industrialio amd_pmc hid_sensor_iio_common evdev hid_multitouch serio_raw mt7921e mt7921_common mt792x_lib mt76_connac_lib mt76
[T34926]  mac80211 libarc4 cfg80211 rfkill msr fuse nvme_fabrics efi_pstore configfs efivarfs autofs4 ext4 crc32c_generic mbcache jbd2 usbhid amdgpu i2c_algo_bit drm_ttm_helper xhci_pci ttm drm_exec drm_suballoc_helper xhci_hcd amdxcp drm_buddy hid_sensor_hub usbcore mfd_core nvme gpu_sched i2c_piix4 hid_generic crc32c_intel psmouse drm_display_helper amd_sfh usb_common i2c_smbus nvme_core crc16 r8169 i2c_hid_acpi i2c_hid hid i2c_designware_platform i2c_designware_core
[T34926] ---[ end trace 0000000000000000 ]---



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: commit 81106b7e0b13 can break asm_int80_emulation on x86_64
@ 2024-08-09 14:53 Bert Karwatzki
  2024-08-09 18:17 ` Linus Torvalds
  0 siblings, 1 reply; 5+ messages in thread
From: Bert Karwatzki @ 2024-08-09 14:53 UTC (permalink / raw)
  To: mingo
  Cc: Bert Karwatzki, akpm, Oleg Nesterov, Andy Lutomirski,
	Borislav Petkov, Fenghua Yu, H . Peter Anvin, Linus Torvalds,
	Dave Hansen, Thomas Gleixner, Uros Bizjak, linux-kernel, peterz

I did some experimentation on the bug with the help of the following patch:

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index bcbbb433cece..70064da40f9d 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1212,6 +1212,7 @@ static int copy_from_buffer(void *dst, unsigned int offset, unsigned int size,
 	if (kbuf) {
 		memcpy(dst, kbuf + offset, size);
 	} else {
+		printk(KERN_INFO "%s: calling copy_from_user with to = %px from = %px, n = 0x%x\n", __func__, dst, ubuf + offset, size);
 		if (copy_from_user(dst, ubuf + offset, size))
 			return -EFAULT;
 	}
@@ -1257,6 +1258,8 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
 	int i;

 	offset = offsetof(struct xregs_state, header);
+	printk(KERN_INFO "%s %d: calling copy_from buffer with offset = 0x%x, size = 0x%lx\n",
+			__func__, __LINE__, offset, sizeof(hdr));
 	if (copy_from_buffer(&hdr, offset, sizeof(hdr), kbuf, ubuf))
 		return -EFAULT;

@@ -1269,6 +1272,8 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
 		u32 mxcsr[2];

 		offset = offsetof(struct fxregs_state, mxcsr);
+		printk(KERN_INFO "%s %d: calling copy_from buffer with offset = 0x%x, size = 0x%lx\n",
+				__func__, __LINE__, offset, sizeof(mxcsr));
 		if (copy_from_buffer(mxcsr, offset, sizeof(mxcsr), kbuf, ubuf))
 			return -EFAULT;

@@ -1292,6 +1297,8 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
 			offset = xstate_offsets[i];
 			size = xstate_sizes[i];

+			printk(KERN_INFO "%s %d: calling copy_from buffer %d with offset = 0x%x, size = 0x%x, dst = %px, kbuf = %px, ubuf = %px\n",
+					__func__, __LINE__, i, offset, size, dst, kbuf, ubuf);
 			if (copy_from_buffer(dst, offset, size, kbuf, ubuf))
 				return -EFAULT;
 		}
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
index 1bab93d049df..23b228804289 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
@@ -1183,7 +1183,7 @@ static void mt7921_ipv6_addr_change(struct ieee80211_hw *hw,
 				    struct inet6_dev *idev)
 {
 	struct mt792x_vif *mvif = (struct mt792x_vif *)vif->drv_priv;
-	struct mt792x_dev *dev = mvif->phy->dev;
+	struct mt792x_dev *dev = mt792x_hw_dev(hw);
 	struct inet6_ifaddr *ifa;
 	struct in6_addr ns_addrs[IEEE80211_BSS_ARP_ADDR_LIST_LEN];
 	struct sk_buff *skb;
diff --git a/mm/slub.c b/mm/slub.c
index 513f0fb80f1b..3a62bf2f355d 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5636,6 +5636,10 @@ void __check_heap_object(const void *ptr, unsigned long n,
 	    n <= s->useroffset - offset + s->usersize)
 		return;

+	printk(KERN_INFO "%s: ptr = %px slab = %px s = %px\n", __func__, ptr, slab, s);
+	printk(KERN_INFO "%s: offset = 0x%x s->useroffset = 0x%x\n", __func__, offset, s->useroffset);
+	printk(KERN_INFO "%s: offset - s->useroffset = 0x%x s->usersize = 0x%x\n", __func__, offset - s->useroffset, s->usersize);
+	printk(KERN_INFO "%s: n = 0x%lx s->useroffset - offset + s->usersize = 0x%x\n", __func__, n, s->useroffset - offset + s->usersize);
 	usercopy_abort("SLUB object", s->name, to_user, offset, n);
 }
 #endif /* CONFIG_HARDENED_USERCOPY */

which gives the following output (before the usual backtrace) :

[  223.785491] [  T46217] copy_uabi_to_xstate 1261: calling copy_from buffer with offset = 0x200, size = 0x40
[  223.785501] [  T46217] copy_from_buffer: calling copy_from_user with to = ffffa85f5387fd58 from = 000000003ffef840, n = 0x40
[  223.785506] [  T46217] copy_uabi_to_xstate 1275: calling copy_from buffer with offset = 0x18, size = 0x8
[  223.785509] [  T46217] copy_from_buffer: calling copy_from_user with to = ffffa85f5387fd50 from = 000000003ffef658, n = 0x8
[  223.785512] [  T46217] copy_uabi_to_xstate 1300: calling copy_from buffer 0 with offset = 0x0, size = 0xa0, dst = ffff8c819c239b80, kbuf = 0000000000000000, ubuf = 000000003ffef640
[  223.785516] [  T46217] copy_from_buffer: calling copy_from_user with to = ffff8c819c239b80 from = 000000003ffef640, n = 0xa0
[  223.785520] [  T46217] __check_heap_object: ptr = ffff8c819c239b80 slab = ffffd5368c708e00 s = ffff8c7f800d1400
[  223.785523] [  T46217] __check_heap_object: offset = 0xc00 s->useroffset = 0x0
[  223.785525] [  T46217] __check_heap_object: offset - s->useroffset = 0xc00 s->usersize = 0x0 FIXME?
[  223.785528] [  T46217] __check_heap_object: n = 0xa0 s->useroffset - offset + s->usersize = 0xfffff400
[  223.785530] [  T46217] usercopy: Kernel memory overwrite attempt detected to SLUB object 'task_struct' (offset 3072, size 160)!
[  223.785545] [  T46217] ------------[ cut here ]------------
[  223.785547] [  T46217] kernel BUG at mm/usercopy.c:102!

So the problem seems to be that the kmem_cache object *s has usersize 0. This
should be impossible in theory as kmem_cache_create_usercopy() should print
a warning in case of (!usersize && useroffset).

Bert Karwatzki


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: commit 81106b7e0b13 can break asm_int80_emulation on x86_64
  2024-08-09 14:53 Bert Karwatzki
@ 2024-08-09 18:17 ` Linus Torvalds
  2024-08-09 23:04   ` Bert Karwatzki
  0 siblings, 1 reply; 5+ messages in thread
From: Linus Torvalds @ 2024-08-09 18:17 UTC (permalink / raw)
  To: Bert Karwatzki
  Cc: mingo, akpm, Oleg Nesterov, Andy Lutomirski, Borislav Petkov,
	Fenghua Yu, H . Peter Anvin, Dave Hansen, Thomas Gleixner,
	Uros Bizjak, linux-kernel, peterz

On Fri, 9 Aug 2024 at 07:53, Bert Karwatzki <spasswolf@web.de> wrote:
>
> So the problem seems to be that the kmem_cache object *s has usersize 0. This
> should be impossible in theory as kmem_cache_create_usercopy() should print
> a warning in case of (!usersize && useroffset).

Following along from your original report:

  usercopy: Kernel memory overwrite attempt detected to SLUB object
'task_struct' (offset 3072, size 160)!

IOW, the user copy code is unhappy about copying data from user mode
into the task_struct allocation.

Anyway, on x86-64 with that commit we now just do

  arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
   {
        *offset = 0;
        *size = 0;
  }

and that seems to be the bug.

It's still allocated together with that 'struct task_struct', even if
it's no longer inside the 'struct thread_struct'.

Ingo? I think it needs to be something like

        *offset = sizeof(struct task_struct)
                - offsetof(struct task_struct, thread)
                + offsetof(struct fpu, __fpstate.regs);
        *size = fpu_kernel_cfg.default_size;

and the commit message of

    The fpu_thread_struct_whitelist() quirk to hardened usercopy can be removed,
    now that the FPU structure is not embedded in the task struct anymore, which
    reduces text footprint a bit.

is clearly not true. No, it's not embedded in 'struct task_struct',
but it *is* still allocated together with it in the same slab if I
read the code correctly (ie this part

  static void __init fpu__init_task_struct_size(void)
  {
        int task_size = sizeof(struct task_struct);

        task_size += sizeof(struct fpu);
        ..
        arch_task_struct_size = task_size;


is because it's still all one single slab allocation.

               Linus

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: commit 81106b7e0b13 can break asm_int80_emulation on x86_64
  2024-08-09 18:17 ` Linus Torvalds
@ 2024-08-09 23:04   ` Bert Karwatzki
  2024-08-11 12:36     ` Bert Karwatzki
  0 siblings, 1 reply; 5+ messages in thread
From: Bert Karwatzki @ 2024-08-09 23:04 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: mingo, akpm, Oleg Nesterov, Andy Lutomirski, Borislav Petkov,
	Fenghua Yu, H . Peter Anvin, Dave Hansen, Thomas Gleixner,
	Uros Bizjak, linux-kernel, peterz, spasswolf

Am Freitag, dem 09.08.2024 um 11:17 -0700 schrieb Linus Torvalds:
> On Fri, 9 Aug 2024 at 07:53, Bert Karwatzki <spasswolf@web.de> wrote:
> >
> > So the problem seems to be that the kmem_cache object *s has usersize 0. This
> > should be impossible in theory as kmem_cache_create_usercopy() should print
> > a warning in case of (!usersize && useroffset).
>
> Following along from your original report:
>
>   usercopy: Kernel memory overwrite attempt detected to SLUB object
> 'task_struct' (offset 3072, size 160)!
>
> IOW, the user copy code is unhappy about copying data from user mode
> into the task_struct allocation.
>
> Anyway, on x86-64 with that commit we now just do
>
>   arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
>    {
>         *offset = 0;
>         *size = 0;
>   }
>
> and that seems to be the bug.
>
> It's still allocated together with that 'struct task_struct', even if
> it's no longer inside the 'struct thread_struct'.
>
> Ingo? I think it needs to be something like
>
>         *offset = sizeof(struct task_struct)
>                 - offsetof(struct task_struct, thread)
>                 + offsetof(struct fpu, __fpstate.regs);
>         *size = fpu_kernel_cfg.default_size;
>
> and the commit message of
>
>     The fpu_thread_struct_whitelist() quirk to hardened usercopy can be removed,
>     now that the FPU structure is not embedded in the task struct anymore, which
>     reduces text footprint a bit.
>
> is clearly not true. No, it's not embedded in 'struct task_struct',
> but it *is* still allocated together with it in the same slab if I
> read the code correctly (ie this part
>
>   static void __init fpu__init_task_struct_size(void)
>   {
>         int task_size = sizeof(struct task_struct);
>
>         task_size += sizeof(struct fpu);
>         ..
>         arch_task_struct_size = task_size;
>
>
> is because it's still all one single slab allocation.
>
>                Linus

Yes, this seems to be the problem. As the old code (from linux-5.16-rc1 to
linux-6.11-rc2) used this

void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
{
	*offset = offsetof(struct thread_struct, fpu.__fpstate.regs);
	*size = fpu_kernel_cfg.default_size;
}

I tried this as a potential solution:

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index bd0621210f63..f1d713de6dba 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -516,8 +516,9 @@ extern struct fpu *x86_task_fpu(struct task_struct *task);
 static inline void
 arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
 {
-       *offset = 0;
-       *size = 0;
+       *offset = sizeof(struct thread_struct)
+               + offsetof(struct fpu, __fpstate.regs);
+       *size = fpu_kernel_cfg.default_size;
 }

 static inline void

and it seems to solve the problem (debug output from my previous printk patch):

[  T57380] copy_uabi_to_xstate 1261: calling copy_from buffer with offset =
0x200, size = 0x40
[  T57380] copy_from_buffer: calling copy_from_user with to = ffffadf495127d58
from = 000000003ffef840, n = 0x40
[  T57380] copy_uabi_to_xstate 1275: calling copy_from buffer with offset =
0x18, size = 0x8
[  T57380] copy_from_buffer: calling copy_from_user with to = ffffadf495127d50
from = 000000003ffef658, n = 0x8
[  T57380] copy_uabi_to_xstate 1300: calling copy_from buffer 0 with offset =
0x0, size = 0xa0, dst = ffff90d285ec7880, kbuf = 0000000000000000, ubuf =
000000003ffef640
[  T57380] copy_from_buffer: calling copy_from_user with to = ffff90d285ec7880
from = 000000003ffef640, n = 0xa0

Here the bug would happen in linux-next-20240806 which seems to be avoided by
the patch.

[  T57380] copy_uabi_to_xstate 1300: calling copy_from buffer 1 with offset =
0xa0, size = 0x100, dst = ffff90d285ec7920, kbuf = 0000000000000000, ubuf =
000000003ffef640
[  T57380] copy_from_buffer: calling copy_from_user with to = ffff90d285ec7920
from = 000000003ffef6e0, n = 0x100

Bert Karwatzki


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: commit 81106b7e0b13 can break asm_int80_emulation on x86_64
  2024-08-09 23:04   ` Bert Karwatzki
@ 2024-08-11 12:36     ` Bert Karwatzki
  0 siblings, 0 replies; 5+ messages in thread
From: Bert Karwatzki @ 2024-08-11 12:36 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: mingo, akpm, Oleg Nesterov, Andy Lutomirski, Borislav Petkov,
	Fenghua Yu, H . Peter Anvin, Dave Hansen, Thomas Gleixner,
	Uros Bizjak, linux-kernel, peterz, spasswolf

Am Samstag, dem 10.08.2024 um 01:04 +0200 schrieb Bert Karwatzki:
> Am Freitag, dem 09.08.2024 um 11:17 -0700 schrieb Linus Torvalds:
> > On Fri, 9 Aug 2024 at 07:53, Bert Karwatzki <spasswolf@web.de> wrote:
> > >
> > > So the problem seems to be that the kmem_cache object *s has usersize 0. This
> > > should be impossible in theory as kmem_cache_create_usercopy() should print
> > > a warning in case of (!usersize && useroffset).
> >
> > Following along from your original report:
> >
> >   usercopy: Kernel memory overwrite attempt detected to SLUB object
> > 'task_struct' (offset 3072, size 160)!
> >
> > IOW, the user copy code is unhappy about copying data from user mode
> > into the task_struct allocation.
> >
> > Anyway, on x86-64 with that commit we now just do
> >
> >   arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
> >    {
> >         *offset = 0;
> >         *size = 0;
> >   }
> >
> > and that seems to be the bug.
> >
> > It's still allocated together with that 'struct task_struct', even if
> > it's no longer inside the 'struct thread_struct'.
> >
> > Ingo? I think it needs to be something like
> >
> >         *offset = sizeof(struct task_struct)
> >                 - offsetof(struct task_struct, thread)
> >                 + offsetof(struct fpu, __fpstate.regs);
> >         *size = fpu_kernel_cfg.default_size;
> >
> > and the commit message of
> >
> >     The fpu_thread_struct_whitelist() quirk to hardened usercopy can be removed,
> >     now that the FPU structure is not embedded in the task struct anymore, which
> >     reduces text footprint a bit.
> >
> > is clearly not true. No, it's not embedded in 'struct task_struct',
> > but it *is* still allocated together with it in the same slab if I
> > read the code correctly (ie this part
> >
> >   static void __init fpu__init_task_struct_size(void)
> >   {
> >         int task_size = sizeof(struct task_struct);
> >
> >         task_size += sizeof(struct fpu);
> >         ..
> >         arch_task_struct_size = task_size;
> >
> >
> > is because it's still all one single slab allocation.
> >
> >                Linus
>
> Yes, this seems to be the problem. As the old code (from linux-5.16-rc1 to
> linux-6.11-rc2) used this
>
> void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
> {
> 	*offset = offsetof(struct thread_struct, fpu.__fpstate.regs);
> 	*size = fpu_kernel_cfg.default_size;
> }
>
> I tried this as a potential solution:
>
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index bd0621210f63..f1d713de6dba 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -516,8 +516,9 @@ extern struct fpu *x86_task_fpu(struct task_struct *task);
>  static inline void
>  arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
>  {
> -       *offset = 0;
> -       *size = 0;
> +       *offset = sizeof(struct thread_struct)
> +               + offsetof(struct fpu, __fpstate.regs);
> +       *size = fpu_kernel_cfg.default_size;
>  }
>
>  static inline void
>
> and it seems to solve the problem (debug output from my previous printk patch):
>
> [  T57380] copy_uabi_to_xstate 1261: calling copy_from buffer with offset =
> 0x200, size = 0x40
> [  T57380] copy_from_buffer: calling copy_from_user with to = ffffadf495127d58
> from = 000000003ffef840, n = 0x40
> [  T57380] copy_uabi_to_xstate 1275: calling copy_from buffer with offset =
> 0x18, size = 0x8
> [  T57380] copy_from_buffer: calling copy_from_user with to = ffffadf495127d50
> from = 000000003ffef658, n = 0x8
> [  T57380] copy_uabi_to_xstate 1300: calling copy_from buffer 0 with offset =
> 0x0, size = 0xa0, dst = ffff90d285ec7880, kbuf = 0000000000000000, ubuf =
> 000000003ffef640
> [  T57380] copy_from_buffer: calling copy_from_user with to = ffff90d285ec7880
> from = 000000003ffef640, n = 0xa0
>
> Here the bug would happen in linux-next-20240806 which seems to be avoided by
> the patch.
>
> [  T57380] copy_uabi_to_xstate 1300: calling copy_from buffer 1 with offset =
> 0xa0, size = 0x100, dst = ffff90d285ec7920, kbuf = 0000000000000000, ubuf =
> 000000003ffef640
> [  T57380] copy_from_buffer: calling copy_from_user with to = ffff90d285ec7920
> from = 000000003ffef6e0, n = 0x100
>
> Bert Karwatzki
>

After taking a closer look at the code I realized that my solution only works if
task_struct and thread_struct are not randomized while your solution

        *offset = sizeof(struct task_struct)
                - offsetof(struct task_struct, thread)
                + offsetof(struct fpu, __fpstate.regs);
        *size = fpu_kernel_cfg.default_size;

works in those cases, too. Here's a way to introdce this fix using
fpu_thread_struct_whitelist in arch/x86/kernel/fpu/init.c where we can use
sizeof(task_struct) without including additional headers:

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index bd0621210f63..87472bdce053 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -510,14 +510,12 @@ extern struct fpu *x86_task_fpu(struct task_struct *task);
 # define x86_task_fpu(task)    ((struct fpu *)((void *)(task) +
sizeof(*(task))))
 #endif

-/*
- * X86 doesn't need any embedded-FPU-struct quirks:
- */
+extern void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long
*size);
+
 static inline void
 arch_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
 {
-       *offset = 0;
-       *size = 0;
+       fpu_thread_struct_whitelist(offset, size);
 }

 static inline void
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 16b6611634c3..173670891f69 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -179,6 +179,18 @@ static void __init fpu__init_task_struct_size(void)
        arch_task_struct_size = task_size;
 }

+/*
+ * Whitelist the FPU register state embedded into task_struct for hardened
+ * usercopy.
+ */
+void fpu_thread_struct_whitelist(unsigned long *offset, unsigned long *size)
+{
+       *offset = sizeof(struct task_struct)
+               - offsetof(struct task_struct, thread)
+               + offsetof(struct fpu, __fpstate.regs);
+       *size = fpu_kernel_cfg.default_size;
+}
+
 /*
  * Set up the user and kernel xstate sizes based on the legacy FPU context
size.
  *


Bert Karwatzki

^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-08-11 12:37 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-08  1:57 commit 81106b7e0b13 can break asm_int80_emulation on x86_64 Bert Karwatzki
  -- strict thread matches above, loose matches on Subject: below --
2024-08-09 14:53 Bert Karwatzki
2024-08-09 18:17 ` Linus Torvalds
2024-08-09 23:04   ` Bert Karwatzki
2024-08-11 12:36     ` Bert Karwatzki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox