* 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