Message ID | 20220124023248.475734-1-linux@roeck-us.net (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | hwmon: (nct6775) Fix crash in clear_caseopen | expand |
On Sun, 23 Jan 2022 18:32:48 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > Paweł Marciniak reports the following crash, observed when clearing > the chassis intrusion alarm. > > BUG: kernel NULL pointer dereference, address: 0000000000000028 > PGD 0 P4D 0 > Oops: 0000 [#1] PREEMPT SMP PTI > CPU: 3 PID: 4815 Comm: bash Tainted: G S > 5.16.2-200.fc35.x86_64 #1 Hardware name: To Be Filled By O.E.M. To Be > Filled By O.E.M./Z97 Extreme4, BIOS P2.60A 05/03/2018 RIP: > 0010:clear_caseopen+0x5a/0x120 [nct6775] Code: 68 70 e8 e9 32 b1 e3 > 85 c0 0f 85 d2 00 00 00 48 83 7c 24 ... RSP: 0018:ffffabcb02803dd8 > EFLAGS: 00010246 RAX: 0000000000000000 RBX: 0000000000000002 RCX: > 0000000000000000 RDX: ffff8e8808192880 RSI: 0000000000000000 RDI: > ffff8e87c7509a68 RBP: 0000000000000000 R08: 0000000000000001 R09: > 000000000000000a R10: 000000000000000a R11: f000000000000000 R12: > 000000000000001f R13: ffff8e87c7509828 R14: ffff8e87c7509a68 R15: > ffff8e88494527a0 FS: 00007f4db9151740(0000) > GS:ffff8e8ebfec0000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 > ES: 0000 CR0: 0000000080050033 CR2: 0000000000000028 CR3: > 0000000166b66001 CR4: 00000000001706e0 Call Trace: > <TASK> > kernfs_fop_write_iter+0x11c/0x1b0 > new_sync_write+0x10b/0x180 > vfs_write+0x209/0x2a0 > ksys_write+0x4f/0xc0 > do_syscall_64+0x3b/0x90 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > The problem is that the device passed to clear_caseopen() is the hwmon > device, not the platform device, and the platform data is not set in > the hwmon device. Store the pointer to sio_data in struct > nct6775_data and get if from there if needed. > > Fixes: 2e7b9886968b ("hwmon: (nct6775) Use superio_*() function > pointers in sio_data.") Cc: Denis Pauk <pauk.denis@gmail.com> > Cc: Bernhard Seibold <mail@bernhard-seibold.de> > Reported-by: Paweł Marciniak <pmarciniak@lodz.home.pl> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/hwmon/nct6775.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c > index fd3f91cb01c6..098d12b9ecda 100644 > --- a/drivers/hwmon/nct6775.c > +++ b/drivers/hwmon/nct6775.c > @@ -1175,7 +1175,7 @@ static inline u8 in_to_reg(u32 val, u8 nr) > > struct nct6775_data { > int addr; /* IO base of hw monitor block */ > - int sioreg; /* SIO register address */ > + struct nct6775_sio_data *sio_data; > enum kinds kind; What do you think about remove kind field also from struct nct6775_data? Code has copy of this field inside of the struct nct6775_sio_data. > const char *name; > > @@ -3559,7 +3559,7 @@ clear_caseopen(struct device *dev, struct > device_attribute *attr, const char *buf, size_t count) > { > struct nct6775_data *data = dev_get_drvdata(dev); > - struct nct6775_sio_data *sio_data = dev_get_platdata(dev); > + struct nct6775_sio_data *sio_data = data->sio_data; > int nr = to_sensor_dev_attr(attr)->index - > INTRUSION_ALARM_BASE; unsigned long val; > u8 reg; > @@ -3967,7 +3967,7 @@ static int nct6775_probe(struct platform_device > *pdev) return -ENOMEM; > > data->kind = sio_data->kind; > - data->sioreg = sio_data->sioreg; > + data->sio_data = sio_data; > > if (sio_data->access == access_direct) { > data->addr = res->start; Tested-by: Denis Pauk <pauk.denis@gmail.com> Best regards, Denis.
On 1/24/22 14:21, Denis Pauk wrote: > On Sun, 23 Jan 2022 18:32:48 -0800 > Guenter Roeck <linux@roeck-us.net> wrote: > >> Paweł Marciniak reports the following crash, observed when clearing >> the chassis intrusion alarm. >> >> BUG: kernel NULL pointer dereference, address: 0000000000000028 >> PGD 0 P4D 0 >> Oops: 0000 [#1] PREEMPT SMP PTI >> CPU: 3 PID: 4815 Comm: bash Tainted: G S >> 5.16.2-200.fc35.x86_64 #1 Hardware name: To Be Filled By O.E.M. To Be >> Filled By O.E.M./Z97 Extreme4, BIOS P2.60A 05/03/2018 RIP: >> 0010:clear_caseopen+0x5a/0x120 [nct6775] Code: 68 70 e8 e9 32 b1 e3 >> 85 c0 0f 85 d2 00 00 00 48 83 7c 24 ... RSP: 0018:ffffabcb02803dd8 >> EFLAGS: 00010246 RAX: 0000000000000000 RBX: 0000000000000002 RCX: >> 0000000000000000 RDX: ffff8e8808192880 RSI: 0000000000000000 RDI: >> ffff8e87c7509a68 RBP: 0000000000000000 R08: 0000000000000001 R09: >> 000000000000000a R10: 000000000000000a R11: f000000000000000 R12: >> 000000000000001f R13: ffff8e87c7509828 R14: ffff8e87c7509a68 R15: >> ffff8e88494527a0 FS: 00007f4db9151740(0000) >> GS:ffff8e8ebfec0000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 >> ES: 0000 CR0: 0000000080050033 CR2: 0000000000000028 CR3: >> 0000000166b66001 CR4: 00000000001706e0 Call Trace: >> <TASK> >> kernfs_fop_write_iter+0x11c/0x1b0 >> new_sync_write+0x10b/0x180 >> vfs_write+0x209/0x2a0 >> ksys_write+0x4f/0xc0 >> do_syscall_64+0x3b/0x90 >> entry_SYSCALL_64_after_hwframe+0x44/0xae >> >> The problem is that the device passed to clear_caseopen() is the hwmon >> device, not the platform device, and the platform data is not set in >> the hwmon device. Store the pointer to sio_data in struct >> nct6775_data and get if from there if needed. >> >> Fixes: 2e7b9886968b ("hwmon: (nct6775) Use superio_*() function >> pointers in sio_data.") Cc: Denis Pauk <pauk.denis@gmail.com> >> Cc: Bernhard Seibold <mail@bernhard-seibold.de> >> Reported-by: Paweł Marciniak <pmarciniak@lodz.home.pl> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> drivers/hwmon/nct6775.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c >> index fd3f91cb01c6..098d12b9ecda 100644 >> --- a/drivers/hwmon/nct6775.c >> +++ b/drivers/hwmon/nct6775.c >> @@ -1175,7 +1175,7 @@ static inline u8 in_to_reg(u32 val, u8 nr) >> >> struct nct6775_data { >> int addr; /* IO base of hw monitor block */ >> - int sioreg; /* SIO register address */ >> + struct nct6775_sio_data *sio_data; >> enum kinds kind; > What do you think about remove kind field also from struct > nct6775_data? Code has copy of this field inside of the struct > nct6775_sio_data. I thought about it, but it would add a level of indirection to each access, and it is used for each register access (through is_word_sized). That would make the change pretty expensive in terms of runtime cost. Guenter >> const char *name; >> >> @@ -3559,7 +3559,7 @@ clear_caseopen(struct device *dev, struct >> device_attribute *attr, const char *buf, size_t count) >> { >> struct nct6775_data *data = dev_get_drvdata(dev); >> - struct nct6775_sio_data *sio_data = dev_get_platdata(dev); >> + struct nct6775_sio_data *sio_data = data->sio_data; >> int nr = to_sensor_dev_attr(attr)->index - >> INTRUSION_ALARM_BASE; unsigned long val; >> u8 reg; >> @@ -3967,7 +3967,7 @@ static int nct6775_probe(struct platform_device >> *pdev) return -ENOMEM; >> >> data->kind = sio_data->kind; >> - data->sioreg = sio_data->sioreg; >> + data->sio_data = sio_data; >> >> if (sio_data->access == access_direct) { >> data->addr = res->start; > > Tested-by: Denis Pauk <pauk.denis@gmail.com> > > Best regards, > Denis.
On Mon, 24 Jan 2022 14:32:26 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > On 1/24/22 14:21, Denis Pauk wrote: > > On Sun, 23 Jan 2022 18:32:48 -0800 > > Guenter Roeck <linux@roeck-us.net> wrote: > > > >> Paweł Marciniak reports the following crash, observed when clearing > >> the chassis intrusion alarm. > >> > >> BUG: kernel NULL pointer dereference, address: 0000000000000028 > >> PGD 0 P4D 0 > >> Oops: 0000 [#1] PREEMPT SMP PTI > >> CPU: 3 PID: 4815 Comm: bash Tainted: G S > >> 5.16.2-200.fc35.x86_64 #1 Hardware name: To Be Filled By O.E.M. To > >> Be Filled By O.E.M./Z97 Extreme4, BIOS P2.60A 05/03/2018 RIP: > >> 0010:clear_caseopen+0x5a/0x120 [nct6775] Code: 68 70 e8 e9 32 b1 e3 > >> 85 c0 0f 85 d2 00 00 00 48 83 7c 24 ... RSP: 0018:ffffabcb02803dd8 > >> EFLAGS: 00010246 RAX: 0000000000000000 RBX: 0000000000000002 RCX: > >> 0000000000000000 RDX: ffff8e8808192880 RSI: 0000000000000000 RDI: > >> ffff8e87c7509a68 RBP: 0000000000000000 R08: 0000000000000001 R09: > >> 000000000000000a R10: 000000000000000a R11: f000000000000000 R12: > >> 000000000000001f R13: ffff8e87c7509828 R14: ffff8e87c7509a68 R15: > >> ffff8e88494527a0 FS: 00007f4db9151740(0000) > >> GS:ffff8e8ebfec0000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 > >> ES: 0000 CR0: 0000000080050033 CR2: 0000000000000028 CR3: > >> 0000000166b66001 CR4: 00000000001706e0 Call Trace: > >> <TASK> > >> kernfs_fop_write_iter+0x11c/0x1b0 > >> new_sync_write+0x10b/0x180 > >> vfs_write+0x209/0x2a0 > >> ksys_write+0x4f/0xc0 > >> do_syscall_64+0x3b/0x90 > >> entry_SYSCALL_64_after_hwframe+0x44/0xae > >> > >> The problem is that the device passed to clear_caseopen() is the > >> hwmon device, not the platform device, and the platform data is > >> not set in the hwmon device. Store the pointer to sio_data in > >> struct nct6775_data and get if from there if needed. > >> > >> Fixes: 2e7b9886968b ("hwmon: (nct6775) Use superio_*() function > >> pointers in sio_data.") Cc: Denis Pauk <pauk.denis@gmail.com> > >> Cc: Bernhard Seibold <mail@bernhard-seibold.de> > >> Reported-by: Paweł Marciniak <pmarciniak@lodz.home.pl> > >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> > >> --- > >> drivers/hwmon/nct6775.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c > >> index fd3f91cb01c6..098d12b9ecda 100644 > >> --- a/drivers/hwmon/nct6775.c > >> +++ b/drivers/hwmon/nct6775.c > >> @@ -1175,7 +1175,7 @@ static inline u8 in_to_reg(u32 val, u8 nr) > >> > >> struct nct6775_data { > >> int addr; /* IO base of hw monitor block */ > >> - int sioreg; /* SIO register address */ > >> + struct nct6775_sio_data *sio_data; > >> enum kinds kind; > > What do you think about remove kind field also from struct > > nct6775_data? Code has copy of this field inside of the struct > > nct6775_sio_data. > > I thought about it, but it would add a level of indirection to > each access, and it is used for each register access (through > is_word_sized). That would make the change pretty expensive > in terms of runtime cost. > I see, thank you. > Guenter > > >> const char *name; > >> > >> @@ -3559,7 +3559,7 @@ clear_caseopen(struct device *dev, struct > >> device_attribute *attr, const char *buf, size_t count) > >> { > >> struct nct6775_data *data = dev_get_drvdata(dev); > >> - struct nct6775_sio_data *sio_data = dev_get_platdata(dev); > >> + struct nct6775_sio_data *sio_data = data->sio_data; > >> int nr = to_sensor_dev_attr(attr)->index - > >> INTRUSION_ALARM_BASE; unsigned long val; > >> u8 reg; > >> @@ -3967,7 +3967,7 @@ static int nct6775_probe(struct > >> platform_device *pdev) return -ENOMEM; > >> > >> data->kind = sio_data->kind; > >> - data->sioreg = sio_data->sioreg; > >> + data->sio_data = sio_data; > >> > >> if (sio_data->access == access_direct) { > >> data->addr = res->start; > > > > Tested-by: Denis Pauk <pauk.denis@gmail.com> > > > > Best regards, > > Denis. > Best regards, Denis.
diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c index fd3f91cb01c6..098d12b9ecda 100644 --- a/drivers/hwmon/nct6775.c +++ b/drivers/hwmon/nct6775.c @@ -1175,7 +1175,7 @@ static inline u8 in_to_reg(u32 val, u8 nr) struct nct6775_data { int addr; /* IO base of hw monitor block */ - int sioreg; /* SIO register address */ + struct nct6775_sio_data *sio_data; enum kinds kind; const char *name; @@ -3559,7 +3559,7 @@ clear_caseopen(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct nct6775_data *data = dev_get_drvdata(dev); - struct nct6775_sio_data *sio_data = dev_get_platdata(dev); + struct nct6775_sio_data *sio_data = data->sio_data; int nr = to_sensor_dev_attr(attr)->index - INTRUSION_ALARM_BASE; unsigned long val; u8 reg; @@ -3967,7 +3967,7 @@ static int nct6775_probe(struct platform_device *pdev) return -ENOMEM; data->kind = sio_data->kind; - data->sioreg = sio_data->sioreg; + data->sio_data = sio_data; if (sio_data->access == access_direct) { data->addr = res->start;
Paweł Marciniak reports the following crash, observed when clearing the chassis intrusion alarm. BUG: kernel NULL pointer dereference, address: 0000000000000028 PGD 0 P4D 0 Oops: 0000 [#1] PREEMPT SMP PTI CPU: 3 PID: 4815 Comm: bash Tainted: G S 5.16.2-200.fc35.x86_64 #1 Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z97 Extreme4, BIOS P2.60A 05/03/2018 RIP: 0010:clear_caseopen+0x5a/0x120 [nct6775] Code: 68 70 e8 e9 32 b1 e3 85 c0 0f 85 d2 00 00 00 48 83 7c 24 ... RSP: 0018:ffffabcb02803dd8 EFLAGS: 00010246 RAX: 0000000000000000 RBX: 0000000000000002 RCX: 0000000000000000 RDX: ffff8e8808192880 RSI: 0000000000000000 RDI: ffff8e87c7509a68 RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000000000a R10: 000000000000000a R11: f000000000000000 R12: 000000000000001f R13: ffff8e87c7509828 R14: ffff8e87c7509a68 R15: ffff8e88494527a0 FS: 00007f4db9151740(0000) GS:ffff8e8ebfec0000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000028 CR3: 0000000166b66001 CR4: 00000000001706e0 Call Trace: <TASK> kernfs_fop_write_iter+0x11c/0x1b0 new_sync_write+0x10b/0x180 vfs_write+0x209/0x2a0 ksys_write+0x4f/0xc0 do_syscall_64+0x3b/0x90 entry_SYSCALL_64_after_hwframe+0x44/0xae The problem is that the device passed to clear_caseopen() is the hwmon device, not the platform device, and the platform data is not set in the hwmon device. Store the pointer to sio_data in struct nct6775_data and get if from there if needed. Fixes: 2e7b9886968b ("hwmon: (nct6775) Use superio_*() function pointers in sio_data.") Cc: Denis Pauk <pauk.denis@gmail.com> Cc: Bernhard Seibold <mail@bernhard-seibold.de> Reported-by: Paweł Marciniak <pmarciniak@lodz.home.pl> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/hwmon/nct6775.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)