linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] kobject: Safe return of kobject_get_path with NULL
@ 2021-06-23 14:47 Christian Löhle
  2021-06-23 14:49 ` Christian Löhle
  2021-06-23 15:11 ` gregkh
  0 siblings, 2 replies; 6+ messages in thread
From: Christian Löhle @ 2021-06-23 14:47 UTC (permalink / raw)
  To: gregkh, linux-kernel, Christian Löhle; +Cc: rafael

Prevent NULL dereference within get_kobj_path_length

Calling kobject_get_path could provoke a NULL dereference
if NULL was passed. while fill_kobj_path will return
with a sane 0 for NULL, kobjet_get_path_length did not.

Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
---
 lib/kobject.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/kobject.c b/lib/kobject.c
index ea53b30cf483..735159c13a94 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -130,6 +130,8 @@ static int get_kobj_path_length(struct kobject *kobj)
 {
 	int length = 1;
 	struct kobject *parent = kobj;
+	if (!kobj)
+		return 0;
 
 	/* walk up the ancestors until we hit the one pointing to the
 	 * root.
-- 
2.32.0

Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782


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

* Re: [PATCH] kobject: Safe return of kobject_get_path with NULL
  2021-06-23 14:47 [PATCH] kobject: Safe return of kobject_get_path with NULL Christian Löhle
@ 2021-06-23 14:49 ` Christian Löhle
  2021-06-23 15:12   ` gregkh
  2021-06-23 15:11 ` gregkh
  1 sibling, 1 reply; 6+ messages in thread
From: Christian Löhle @ 2021-06-23 14:49 UTC (permalink / raw)
  To: gregkh, linux-kernel, Christian Löhle; +Cc: rafael

This prevents two Oopses I've encountered.
(Sorry for the older kernel)

[ 4650.202534] general protection fault: 0000 [#1] PREEMPT SMP
[ 4650.202554] Modules linked in: serio_raw atkbd libps2 aesni_intel aes_x86_64 crypto_simd glue_helper ahci libahci i8042 fam15h_power hwmon_vid k10temp scsi_mon ftdi_sio usbserial exfat ext4 crc16 mbcache jbd2 fscrypto sg fuse nls_iso8859_1 nls_cp437 vfat fat nfsv3 nfs_acl nfsv4 dns_resolver nfs lockd grace sunrpc fscache e1000e ptp pps_core i915 intel_gtt usbmon button serio ehci_pci ehci_hcd r8169 mii xhci_pci xhci_hcd libcrc32c crc32c_generic crc32c_intel crc32_pclmul radeon i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm agpgart ata_piix pata_acpi ata_generic libata uas usb_storage sd_mod scsi_mod isofs loop overlay ip_tables x_tables sch_fq_codel tpm_tis tpm_tis_core tpm lpc_ich ie31200_edac mei_me mei shpchp parport_pc battery fan thermal i2c_i801 intel_rapl_perf
[ 4650.202746]  intel_cstate ghash_clmulni_intel cryptd crct10dif_pclmul ppdev parport mousedev input_leds iTCO_wdt iTCO_vendor_support kvm irqbypass evdev mac_hid eeepc_wmi asus_wmi sparse_keymap wmi led_class video coretemp intel_powerclamp x86_pkg_temp_thermal intel_rapl cfg80211 rfkill hid_generic usbhid hid usbcore usb_common
[ 4650.202824] CPU: 0 PID: 20750 Comm: python3 Not tainted 4.13.5-1hyLinux #22
[ 4650.202840] Hardware name: ASUS All Series/Z97-P, BIOS 2905 12/04/2015
[ 4650.202855] task: ffff88020f61cb00 task.stack: ffffc90002814000
[ 4650.202873] RIP: 0010:kobject_get_path+0x30/0xf0
[ 4650.202883] RSP: 0018:ffffc900028179e0 EFLAGS: 00010206
[ 4650.202896] RAX: 0000000000000005 RBX: 000000000000000b RCX: 0000000000000000
[ 4650.202912] RDX: 0000000000001000 RSI: 00000000014000c0 RDI: 0000000000000005
[ 4650.202928] RBP: ffffc90002817a08 R08: ffff880217002c00 R09: ffff88020d66b000
[ 4650.202945] R10: ffffea0008537b00 R11: 0000000000000001 R12: 372e63313a30303a
[ 4650.202961] R13: ffff88020d650080 R14: 00000000014000c0 R15: ffff880216352120
[ 4650.202977] FS:  00007f5119e3a640(0000) GS:ffff88021fa00000(0000) knlGS:0000000000000000
[ 4650.202996] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4650.203009] CR2: 000055a91ad5d130 CR3: 0000000210c09000 CR4: 00000000001406f0
[ 4650.203026] Call Trace:
[ 4650.203035]  kobject_uevent_env+0xcf/0x4e0
[ 4650.203047]  disk_check_events+0xe3/0x140
[ 4650.203057]  disk_clear_events+0x77/0x110
[ 4650.203070]  check_disk_change+0x27/0x70
[ 4650.203082]  sd_open+0x75/0x190 [sd_mod]
[ 4650.203092]  __blkdev_get+0x348/0x430
[ 4650.203102]  ? __follow_mount_rcu.isra.30+0x68/0xe0
[ 4650.203115]  blkdev_get+0x127/0x340
[ 4650.203125]  blkdev_open+0x79/0x90
[ 4650.203135]  do_dentry_open+0x1bc/0x2e0
[ 4650.203146]  ? bd_acquire+0xd0/0xd0
[ 4650.203156]  vfs_open+0x4e/0x80
[ 4650.203164]  path_openat+0x51e/0x13a0
[ 4650.203174]  ? mntput_no_expire+0x2c/0x1a0
[ 4650.203184]  ? mntput+0x24/0x40
[ 4650.203194]  do_filp_open+0x9b/0x110
[ 4650.203204]  ? __check_object_size+0xb1/0x190
[ 4650.203215]  ? __alloc_fd+0xb2/0x160
[ 4650.203225]  do_sys_open+0x1ba/0x250
[ 4650.203233]  ? do_sys_open+0x1ba/0x250
[ 4650.203243]  SyS_openat+0x14/0x20
[ 4650.203253]  entry_SYSCALL_64_fastpath+0x1a/0xa5
[ 4650.203265] RIP: 0033:0x7f511eb0efe4
[ 4650.203274] RSP: 002b:00007f5119e37830 EFLAGS: 00000293 ORIG_RAX: 0000000000000101
[ 4650.203292] RAX: ffffffffffffffda RBX: 00007f5104006b40 RCX: 00007f511eb0efe4
[ 4650.203308] RDX: 0000000000080000 RSI: 00007f511b9a0650 RDI: 00000000ffffff9c
[ 4650.203324] RBP: 00007f511a70a298 R08: 0000000000000000 R09: 000000000000007c
[ 4650.203340] R10: 0000000000000000 R11: 0000000000000293 R12: 0000556257edd1b0
[ 4650.204199] R13: 00007f511d446ab2 R14: ffffffffffffffff R15: 0000000000000000
[ 4650.205027] Code: 57 41 56 41 89 f6 41 55 49 89 fd 41 54 49 89 fc 53 bb 01 00 00 00 eb 13 e8 9e 8a 00 00 4d 8b 64 24 18 8d 5c 03 01 4d 85 e4 74 1a <49> 8b 3c 24 48 85 ff 75 e4 45 31 e4 5b 4c 89 e0 41 5c 41 5d 41 
[ 4650.206990] RIP: kobject_get_path+0x30/0xf0 RSP: ffffc900028179e0



[273327.641637] general protection fault: 0000 [#1] PREEMPT SMP
[273327.641656] Modules linked in: serio_raw atkbd libps2 aesni_intel aes_x86_64 crypto_simd glue_helper ahci libahci i8042 fam15h_power hwmon_vid k10temp scsi_mon ftdi_sio usbserial exfat ext4 crc16 mbcache jbd2 fscrypto sg fuse nls_iso8859_1 nls_cp437 vfat fat nfsv3 nfs_acl nfsv4 dns_resolver nfs lockd grace sunrpc fscache e1000e ptp pps_core i915 intel_gtt usbmon button serio ehci_pci ehci_hcd r8169 mii xhci_pci xhci_hcd libcrc32c crc32c_generic crc32c_intel crc32_pclmul radeon i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm agpgart ata_piix pata_acpi ata_generic libata uas usb_storage sd_mod scsi_mod isofs loop overlay ip_tables x_tables sch_fq_codel tpm_tis tpm_tis_core tpm lpc_ich ie31200_edac mei_me mei shpchp parport_pc battery fan thermal i2c_i801 intel_rapl_perf
[273327.641856]  intel_cstate ghash_clmulni_intel cryptd crct10dif_pclmul ppdev parport mousedev input_leds iTCO_wdt iTCO_vendor_support kvm irqbypass evdev mac_hid eeepc_wmi asus_wmi sparse_keymap wmi led_class video coretemp intel_powerclamp x86_pkg_temp_thermal intel_rapl cfg80211 rfkill hid_generic usbhid hid usbcore usb_common
[273327.641931] CPU: 3 PID: 1636823 Comm: python3 Not tainted 4.13.5-1hyLinux #22
[273327.641947] Hardware name: ASUS All Series/Z97-P, BIOS 2905 12/04/2015
[273327.641962] task: ffff88020bc15a00 task.stack: ffffc9000399c000
[273327.641979] RIP: 0010:kobject_get_path+0x30/0xf0
[273327.641990] RSP: 0018:ffffc9000399f9e0 EFLAGS: 00010206
[273327.642002] RAX: 0000000000000005 RBX: 000000000000000b RCX: 0000000000000000
[273327.642019] RDX: 0000000000001000 RSI: 00000000014000c0 RDI: 0000000000000005
[273327.642035] RBP: ffffc9000399fa08 R08: ffff880217002c00 R09: ffff8802115cd000
[273327.642050] R10: ffffea00084b7b00 R11: 0000000000000001 R12: 372e63313a30303a
[273327.642067] R13: ffff88020f8ea080 R14: 00000000014000c0 R15: ffff880216352000
[273327.642083] FS:  00007f6fb4be0640(0000) GS:ffff88021fb80000(0000) knlGS:0000000000000000
[273327.642101] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[273327.642114] CR2: 00007f2762bb3d80 CR3: 0000000214fab000 CR4: 00000000001406e0
[273327.642130] Call Trace:
[273327.642139]  kobject_uevent_env+0xcf/0x4e0
[273327.642151]  disk_check_events+0xe3/0x140
[273327.642161]  disk_clear_events+0x77/0x110
[273327.642174]  check_disk_change+0x27/0x70
[273327.642185]  sd_open+0x75/0x190 [sd_mod]
[273327.642195]  __blkdev_get+0x348/0x430
[273327.642206]  ? __follow_mount_rcu.isra.30+0x68/0xe0
[273327.642218]  blkdev_get+0x127/0x340
[273327.642228]  blkdev_open+0x79/0x90
[273327.642249]  do_dentry_open+0x1bc/0x2e0
[273327.642259]  ? bd_acquire+0xd0/0xd0
[273327.642269]  vfs_open+0x4e/0x80
[273327.642279]  path_openat+0x51e/0x13a0
[273327.642289]  ? mntput_no_expire+0x2c/0x1a0
[273327.642299]  ? mntput+0x24/0x40
[273327.642310]  do_filp_open+0x9b/0x110
[273327.642321]  ? __check_object_size+0xb1/0x190
[273327.642332]  ? __alloc_fd+0xb2/0x160
[273327.642342]  do_sys_open+0x1ba/0x250
[273327.642352]  ? do_sys_open+0x1ba/0x250
[273327.642371]  SyS_openat+0x14/0x20
[273327.642392]  entry_SYSCALL_64_fastpath+0x1a/0xa5
[273327.642403] RIP: 0033:0x7f6fb9a30fe4
[273327.642412] RSP: 002b:00007f6fb4bdd830 EFLAGS: 00000293 ORIG_RAX: 0000000000000101
[273327.642430] RAX: ffffffffffffffda RBX: 00007f6fa400a840 RCX: 00007f6fb9a30fe4
[273327.642447] RDX: 0000000000080000 RSI: 00007f6fb6f5da10 RDI: 00000000ffffff9c
[273327.642463] RBP: 00007f6fb846c928 R08: 0000000000000000 R09: 000000000000007c
[273327.642480] R10: 0000000000000000 R11: 0000000000000293 R12: 0000562253e6be10
[273327.643330] R13: 00007f6fb8efbb62 R14: ffffffffffffffff R15: 0000000000000000
[273327.644179] Code: 57 41 56 41 89 f6 41 55 49 89 fd 41 54 49 89 fc 53 bb 01 00 00 00 eb 13 e8 9e 8a 00 00 4d 8b 64 24 18 8d 5c 03 01 4d 85 e4 74 1a <49> 8b 3c 24 48 85 ff 75 e4 45 31 e4 5b 4c 89 e0 41 5c 41 5d 41 
[273327.645958] RIP: kobject_get_path+0x30/0xf0 RSP: ffffc9000399f9e0
Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782


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

* Re: [PATCH] kobject: Safe return of kobject_get_path with NULL
  2021-06-23 14:47 [PATCH] kobject: Safe return of kobject_get_path with NULL Christian Löhle
  2021-06-23 14:49 ` Christian Löhle
@ 2021-06-23 15:11 ` gregkh
  2021-06-24  6:42   ` Christian Löhle
  1 sibling, 1 reply; 6+ messages in thread
From: gregkh @ 2021-06-23 15:11 UTC (permalink / raw)
  To: Christian Löhle; +Cc: linux-kernel, rafael

On Wed, Jun 23, 2021 at 02:47:35PM +0000, Christian Löhle wrote:
> Prevent NULL dereference within get_kobj_path_length
> 
> Calling kobject_get_path could provoke a NULL dereference
> if NULL was passed. while fill_kobj_path will return
> with a sane 0 for NULL, kobjet_get_path_length did not.

Who passes NULL into that function?  Shouldn't that be fixed first?

> 
> Signed-off-by: Christian Loehle <cloehle@hyperstone.com>
> ---
>  lib/kobject.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index ea53b30cf483..735159c13a94 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -130,6 +130,8 @@ static int get_kobj_path_length(struct kobject *kobj)
>  {
>  	int length = 1;
>  	struct kobject *parent = kobj;
> +	if (!kobj)
> +		return 0;
>  
>  	/* walk up the ancestors until we hit the one pointing to the
>  	 * root.
> -- 
> 2.32.0
> 
> Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
> Managing Directors: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
> 

Pleaase always run your patches through checkpatch.pl so you do not get
maintainers asking you to use checkpatch.pl...

thanks,

greg k-h

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

* Re: [PATCH] kobject: Safe return of kobject_get_path with NULL
  2021-06-23 14:49 ` Christian Löhle
@ 2021-06-23 15:12   ` gregkh
  0 siblings, 0 replies; 6+ messages in thread
From: gregkh @ 2021-06-23 15:12 UTC (permalink / raw)
  To: Christian Löhle; +Cc: linux-kernel, rafael

On Wed, Jun 23, 2021 at 02:49:57PM +0000, Christian Löhle wrote:
> This prevents two Oopses I've encountered.
> (Sorry for the older kernel)
> 
> [ 4650.202534] general protection fault: 0000 [#1] PREEMPT SMP
> [ 4650.202554] Modules linked in: serio_raw atkbd libps2 aesni_intel aes_x86_64 crypto_simd glue_helper ahci libahci i8042 fam15h_power hwmon_vid k10temp scsi_mon ftdi_sio usbserial exfat ext4 crc16 mbcache jbd2 fscrypto sg fuse nls_iso8859_1 nls_cp437 vfat fat nfsv3 nfs_acl nfsv4 dns_resolver nfs lockd grace sunrpc fscache e1000e ptp pps_core i915 intel_gtt usbmon button serio ehci_pci ehci_hcd r8169 mii xhci_pci xhci_hcd libcrc32c crc32c_generic crc32c_intel crc32_pclmul radeon i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm agpgart ata_piix pata_acpi ata_generic libata uas usb_storage sd_mod scsi_mod isofs loop overlay ip_tables x_tables sch_fq_codel tpm_tis tpm_tis_core tpm lpc_ich ie31200_edac mei_me mei shpchp parport_pc battery fan thermal i2c_i801 intel_rapl_perf
> [ 4650.202746]  intel_cstate ghash_clmulni_intel cryptd crct10dif_pclmul ppdev parport mousedev input_leds iTCO_wdt iTCO_vendor_support kvm irqbypass evdev mac_hid eeepc_wmi asus_wmi sparse_keymap wmi led_class video coretemp intel_powerclamp x86_pkg_temp_thermal intel_rapl cfg80211 rfkill hid_generic usbhid hid usbcore usb_common
> [ 4650.202824] CPU: 0 PID: 20750 Comm: python3 Not tainted 4.13.5-1hyLinux #22

Lots of things have changed since this old kernel version.  Can you see
if this is still needed on Linus's tree?

thanks,

greg k-h

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

* Re: [PATCH] kobject: Safe return of kobject_get_path with NULL
  2021-06-23 15:11 ` gregkh
@ 2021-06-24  6:42   ` Christian Löhle
  2021-06-24  9:02     ` gregkh
  0 siblings, 1 reply; 6+ messages in thread
From: Christian Löhle @ 2021-06-24  6:42 UTC (permalink / raw)
  To: gregkh, linux-kernel

Hey Greg,

>> Prevent NULL dereference within get_kobj_path_length
>> 
>> Calling kobject_get_path could provoke a NULL dereference
>> if NULL was passed. while fill_kobj_path will return
>> with a sane 0 for NULL, kobjet_get_path_length did not.
>
>Who passes NULL into that function?  Shouldn't that be fixed first?

It seems to me like here specifically it was a sd_open on some no longer
existing device. I agree, but could not find a fix for that, and even if, it might
not have been in the current tree.
But when looking at the kobject code it felt like it was meant to be safe for
NULL, (like any parent in the tree can be NULL), but the do while does hide that
a bit.
So is it not meant to be safe?
I will try to find the sd_open issue some more, but cannot reproduce the issue
consistently enough right now.


>Pleaase always run your patches through checkpatch.pl so you do not get
>maintainers asking you to use checkpatch.pl...

I did, so please tell me what part bothers you, so I can get that fixed, either in v2
or maybe even in checkpatch.pl?
(Only thing I spotted now is the kobjet typo)

Regards,
Christian
Hyperstone GmbH | Line-Eid-Strasse 3 | 78467 Konstanz
Managing Directors: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782


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

* Re: [PATCH] kobject: Safe return of kobject_get_path with NULL
  2021-06-24  6:42   ` Christian Löhle
@ 2021-06-24  9:02     ` gregkh
  0 siblings, 0 replies; 6+ messages in thread
From: gregkh @ 2021-06-24  9:02 UTC (permalink / raw)
  To: Christian Löhle; +Cc: linux-kernel

On Thu, Jun 24, 2021 at 06:42:41AM +0000, Christian Löhle wrote:
> Hey Greg,
> 
> >> Prevent NULL dereference within get_kobj_path_length
> >> 
> >> Calling kobject_get_path could provoke a NULL dereference
> >> if NULL was passed. while fill_kobj_path will return
> >> with a sane 0 for NULL, kobjet_get_path_length did not.
> >
> >Who passes NULL into that function?  Shouldn't that be fixed first?
> 
> It seems to me like here specifically it was a sd_open on some no longer
> existing device. I agree, but could not find a fix for that, and even if, it might
> not have been in the current tree.
> But when looking at the kobject code it felt like it was meant to be safe for
> NULL, (like any parent in the tree can be NULL), but the do while does hide that
> a bit.
> So is it not meant to be safe?

Not always, no.  It's better to fix the root problem and not paper over
it by doing something like this.

> I will try to find the sd_open issue some more, but cannot reproduce the issue
> consistently enough right now.
> 
> 
> >Pleaase always run your patches through checkpatch.pl so you do not get
> >maintainers asking you to use checkpatch.pl...
> 
> I did, so please tell me what part bothers you, so I can get that fixed, either in v2
> or maybe even in checkpatch.pl?
> (Only thing I spotted now is the kobjet typo)

You should put a blank line after variable definitions and before the
code logic.  Normally checkpatch catches that, odd it did not here...

thanks,

greg k-h

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

end of thread, other threads:[~2021-06-24  9:02 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-23 14:47 [PATCH] kobject: Safe return of kobject_get_path with NULL Christian Löhle
2021-06-23 14:49 ` Christian Löhle
2021-06-23 15:12   ` gregkh
2021-06-23 15:11 ` gregkh
2021-06-24  6:42   ` Christian Löhle
2021-06-24  9:02     ` gregkh

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).