In function 'neo_intr', the driver uses 'ch->ch_equeue' and
'ch->ch_reuque'. These two pointers are initialized in 'jsm_tty_open',
but the interrupt handler 'neo_intr' has been registered in the probe
progress. If 'jsm_tty_open' has not been called at this time, it will
cause null pointer dereference.
Once the driver registers the interrupt handler, the driver should be
ready to handle it.
Fix this by allocating the memory at probe time and not at open time.
This log reveals it:
[ 12.771912] BUG: kernel NULL pointer dereference, address: 0000000000000000
[ 12.774932] #PF: supervisor write access in kernel mode
[ 12.775314] #PF: error_code(0x0002) - not-present page
[ 12.775689] PGD 0 P4D 0
[ 12.775881] Oops: 0002 [#1] PREEMPT SMP PTI
[ 12.776212] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 5.12.4-g70e7f0549188-dirty #106
[ 12.776803] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
[ 12.777627] RIP: 0010:memcpy_fromio+0x75/0xa0
[ 12.777983] Code: e9 02 f3 a5 41 f6 c5 02 74 02 66 a5 41 f6 c5 01 74 01 a4 e8 5d 4e 6b ff 5b 41 5c 41 5d 5d c3 e8 51 4e 6b ff 4c 89 e7 48 89 de <a4> 49 89 fc 48 89 f3 49 83 ed 01 eb a4 e8 39 4e 6b ff 4c 89 e7 48
[ 12.779377] RSP: 0018:ffffc90000118db0 EFLAGS: 00010046
[ 12.779771] RAX: ffff888100258000 RBX: ffffc90007d0010f RCX: 0000000000000000
[ 12.780298] RDX: 0000000000000000 RSI: ffffc90007d0010f RDI: 0000000000000000
[ 12.780820] RBP: ffffc90000118dc8 R08: 0000000000000000 R09: 0000000000000000
[ 12.781359] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
[ 12.781928] R13: 0000000000000001 R14: 0000000007d0009e R15: 0000000000000000
[ 12.782453] FS: 0000000000000000(0000) GS:ffff88817bc80000(0000) knlGS:0000000000000000
[ 12.783067] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 12.783499] CR2: 0000000000000000 CR3: 0000000005e2e000 CR4: 00000000000006e0
[ 12.784051] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 12.784579] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 12.785105] Call Trace:
[ 12.785295] <IRQ>
[ 12.785457] neo_copy_data_from_uart_to_queue+0x2f7/0x4e0
[ 12.785871] neo_parse_isr.part.4+0x175/0x4d0
[ 12.786217] neo_intr+0x188/0x7a0
[ 12.786478] __handle_irq_event_percpu+0x53/0x3e0
[ 12.786831] handle_irq_event_percpu+0x35/0x90
[ 12.787157] handle_irq_event+0x39/0x60
[ 12.787458] handle_fasteoi_irq+0xc2/0x1d0
[ 12.787763] __common_interrupt+0x7f/0x150
[ 12.788071] common_interrupt+0xb4/0xd0
[ 12.788358] </IRQ>
[ 12.788532] asm_common_interrupt+0x1e/0x40
[ 12.788853] RIP: 0010:native_safe_halt+0x17/0x20
[ 12.789199] Code: 07 0f 00 2d 0b ab 50 00 f4 5d c3 0f 1f 84 00 00 00 00 00 8b 05 f2 11 f7 01 55 48 89 e5 85 c0 7e 07 0f 00 2d eb aa 50 00 fb f4 <5d> c3 cc cc cc cc cc cc cc 55 48 89 e5 e8 67 53 ff ff 8b 0d e9 dc
[ 12.790581] RSP: 0018:ffffc9000008fe90 EFLAGS: 00000246
[ 12.790975] RAX: 0000000000000000 RBX: 0000000000000002 RCX: 0000000000000000
[ 12.791502] RDX: 0000000000000000 RSI: ffffffff859d2c94 RDI: ffffffff8589953e
[ 12.792031] RBP: ffffc9000008fe90 R08: 0000000000000001 R09: 0000000000000001
[ 12.792573] R10: 0000000000000000 R11: 0000000000000001 R12: ffffffff86434488
[ 12.793095] R13: 0000000000000000 R14: 0000000000000000 R15: ffff888100258000
[ 12.793625] default_idle+0x9/0x10
[ 12.793898] arch_cpu_idle+0xa/0x10
[ 12.794159] default_idle_call+0x6e/0x250
[ 12.794462] do_idle+0x1f0/0x2d0
[ 12.794708] cpu_startup_entry+0x18/0x20
[ 12.795008] start_secondary+0x11f/0x160
[ 12.795314] secondary_startup_64_no_verify+0xb0/0xbb
[ 12.795701] Modules linked in:
[ 12.795931] Dumping ftrace buffer:
[ 12.796206] (ftrace buffer empty)
[ 12.796481] CR2: 0000000000000000
[ 12.796741] ---[ end trace 5535b8755359e59f ]---
[ 12.797089] RIP: 0010:memcpy_fromio+0x75/0xa0
[ 12.797417] Code: e9 02 f3 a5 41 f6 c5 02 74 02 66 a5 41 f6 c5 01 74 01 a4 e8 5d 4e 6b ff 5b 41 5c 41 5d 5d c3 e8 51 4e 6b ff 4c 89 e7 48 89 de <a4> 49 89 fc 48 89 f3 49 83 ed 01 eb a4 e8 39 4e 6b ff 4c 89 e7 48
[ 12.798787] RSP: 0018:ffffc90000118db0 EFLAGS: 00010046
[ 12.799175] RAX: ffff888100258000 RBX: ffffc90007d0010f RCX: 0000000000000000
[ 12.799702] RDX: 0000000000000000 RSI: ffffc90007d0010f RDI: 0000000000000000
[ 12.800226] RBP: ffffc90000118dc8 R08: 0000000000000000 R09: 0000000000000000
[ 12.800753] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
[ 12.801287] R13: 0000000000000001 R14: 0000000007d0009e R15: 0000000000000000
[ 12.801809] FS: 0000000000000000(0000) GS:ffff88817bc80000(0000) knlGS:0000000000000000
[ 12.802431] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 12.802862] CR2: 0000000000000000 CR3: 0000000005e2e000 CR4: 00000000000006e0
[ 12.803412] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 12.803932] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 12.804447] Kernel panic - not syncing: Fatal exception in interrupt
[ 12.805067] Dumping ftrace buffer:
[ 12.805315] (ftrace buffer empty)
[ 12.805584] Kernel Offset: disabled
[ 12.805850] Rebooting in 1 seconds..
Signed-off-by: Zheyu Ma <[email protected]>
---
Changes in v4:
- Add error handling when allocating memory fails
Changes in v3:
- Remove useless comments
- Remove uncessary flag changes
- Keep flushing input quques in the open function
Changes in v2:
- Allocate the memory at probe time, instead of simply checking
whether it is a null pointer.
---
drivers/tty/serial/jsm/jsm_tty.c | 59 ++++++++++++--------------------
1 file changed, 22 insertions(+), 37 deletions(-)
diff --git a/drivers/tty/serial/jsm/jsm_tty.c b/drivers/tty/serial/jsm/jsm_tty.c
index 8e42a7682c63..4f17ad3ae0b0 100644
--- a/drivers/tty/serial/jsm/jsm_tty.c
+++ b/drivers/tty/serial/jsm/jsm_tty.c
@@ -195,38 +195,6 @@ static int jsm_tty_open(struct uart_port *port)
/* Get board pointer from our array of majors we have allocated */
brd = channel->ch_bd;
- /*
- * Allocate channel buffers for read/write/error.
- * Set flag, so we don't get trounced on.
- */
- channel->ch_flags |= (CH_OPENING);
-
- /* Drop locks, as malloc with GFP_KERNEL can sleep */
-
- if (!channel->ch_rqueue) {
- channel->ch_rqueue = kzalloc(RQUEUESIZE, GFP_KERNEL);
- if (!channel->ch_rqueue) {
- jsm_dbg(INIT, &channel->ch_bd->pci_dev,
- "unable to allocate read queue buf\n");
- return -ENOMEM;
- }
- }
- if (!channel->ch_equeue) {
- channel->ch_equeue = kzalloc(EQUEUESIZE, GFP_KERNEL);
- if (!channel->ch_equeue) {
- jsm_dbg(INIT, &channel->ch_bd->pci_dev,
- "unable to allocate error queue buf\n");
- return -ENOMEM;
- }
- }
-
- channel->ch_flags &= ~(CH_OPENING);
- /*
- * Initialize if neither terminal is open.
- */
- jsm_dbg(OPEN, &channel->ch_bd->pci_dev,
- "jsm_open: initializing channel in open...\n");
-
/*
* Flush input queues.
*/
@@ -389,11 +357,8 @@ int jsm_tty_init(struct jsm_board *brd)
* interrupt context, and there are no locks held.
*/
brd->channels[i] = kzalloc(sizeof(struct jsm_channel), GFP_KERNEL);
- if (!brd->channels[i]) {
- jsm_dbg(CORE, &brd->pci_dev,
- "%s:%d Unable to allocate memory for channel struct\n",
- __FILE__, __LINE__);
- }
+ if (!brd->channels[i])
+ goto err;
}
}
@@ -420,10 +385,30 @@ int jsm_tty_init(struct jsm_board *brd)
ch->ch_close_delay = 250;
init_waitqueue_head(&ch->ch_flags_wait);
+
+ if (!ch->ch_rqueue) {
+ ch->ch_rqueue = kzalloc(RQUEUESIZE, GFP_KERNEL);
+ if (!ch->ch_rqueue)
+ goto err;
+ }
+ if (!ch->ch_equeue) {
+ ch->ch_equeue = kzalloc(EQUEUESIZE, GFP_KERNEL);
+ if (!ch->ch_equeue)
+ goto err;
+ }
}
jsm_dbg(INIT, &brd->pci_dev, "finish\n");
return 0;
+err:
+ for (i = 0; i < brd->nasync; i++) {
+ if (brd->channels[i]) {
+ kfree(brd->channels[i]->ch_rqueue);
+ kfree(brd->channels[i]->ch_equeue);
+ kfree(brd->channels[i]);
+ }
+ }
+ return -ENOMEM;
}
int jsm_uart_port_init(struct jsm_board *brd)
--
2.17.6
On 05. 07. 21, 14:53, Zheyu Ma wrote:
> In function 'neo_intr', the driver uses 'ch->ch_equeue' and
> 'ch->ch_reuque'. These two pointers are initialized in 'jsm_tty_open',
> but the interrupt handler 'neo_intr' has been registered in the probe
> progress. If 'jsm_tty_open' has not been called at this time, it will
> cause null pointer dereference.
>
> Once the driver registers the interrupt handler, the driver should be
> ready to handle it.
>
> Fix this by allocating the memory at probe time and not at open time.
You are allocating the buffer in jsm_tty_init now. But that is still
called after request_irq() in probe. So care to explain how this helps
exactly? As I understand it, you only made the window much smaller.
Anyway, I'm no expert on jsm, but AFAICS jsm_tty_open first allocates
the buffers, brd->bd_ops->uart_init() / neo_uart_init() clears ier and
only brd->bd_ops->param() / neo_param() enables interrupts on the device
(by ier update and write). So how it comes an interrupt came before
neo_param() in jsm_tty_open was called?
> This log reveals it:
>
> [ 12.771912] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [ 12.774932] #PF: supervisor write access in kernel mode
> [ 12.775314] #PF: error_code(0x0002) - not-present page
> [ 12.775689] PGD 0 P4D 0
> [ 12.775881] Oops: 0002 [#1] PREEMPT SMP PTI
> [ 12.776212] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 5.12.4-g70e7f0549188-dirty #106
> [ 12.776803] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
> [ 12.777627] RIP: 0010:memcpy_fromio+0x75/0xa0
> [ 12.777983] Code: e9 02 f3 a5 41 f6 c5 02 74 02 66 a5 41 f6 c5 01 74 01 a4 e8 5d 4e 6b ff 5b 41 5c 41 5d 5d c3 e8 51 4e 6b ff 4c 89 e7 48 89 de <a4> 49 89 fc 48 89 f3 49 83 ed 01 eb a4 e8 39 4e 6b ff 4c 89 e7 48
> [ 12.779377] RSP: 0018:ffffc90000118db0 EFLAGS: 00010046
> [ 12.779771] RAX: ffff888100258000 RBX: ffffc90007d0010f RCX: 0000000000000000
> [ 12.780298] RDX: 0000000000000000 RSI: ffffc90007d0010f RDI: 0000000000000000
> [ 12.780820] RBP: ffffc90000118dc8 R08: 0000000000000000 R09: 0000000000000000
> [ 12.781359] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
> [ 12.781928] R13: 0000000000000001 R14: 0000000007d0009e R15: 0000000000000000
> [ 12.782453] FS: 0000000000000000(0000) GS:ffff88817bc80000(0000) knlGS:0000000000000000
> [ 12.783067] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 12.783499] CR2: 0000000000000000 CR3: 0000000005e2e000 CR4: 00000000000006e0
> [ 12.784051] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 12.784579] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 12.785105] Call Trace:
> [ 12.785295] <IRQ>
> [ 12.785457] neo_copy_data_from_uart_to_queue+0x2f7/0x4e0
> [ 12.785871] neo_parse_isr.part.4+0x175/0x4d0
> [ 12.786217] neo_intr+0x188/0x7a0
thanks,
--
--
js
suse labs
On Wed, Jul 7, 2021 at 4:14 PM Andy Shevchenko
<[email protected]> wrote:
>
>
>
> On Monday, July 5, 2021, Zheyu Ma <[email protected]> wrote:
>>
>> In function 'neo_intr', the driver uses 'ch->ch_equeue' and
>> 'ch->ch_reuque'. These two pointers are initialized in 'jsm_tty_open',
>> but the interrupt handler 'neo_intr' has been registered in the probe
>> progress. If 'jsm_tty_open' has not been called at this time, it will
>> cause null pointer dereference.
>>
>> Once the driver registers the interrupt handler, the driver should be
>> ready to handle it.
>>
>> Fix this by allocating the memory at probe time and not at open time.
>>
>> This log reveals it:
>
>
>
> When doing commit messages try to avoid tons of noise in them, I.e. here is _a lot of useless lines_ from the log, has to be addressed.
Thanks for your advice, I will learn from other commits.
Regards,
Zheyu Ma
On Wed, Jul 7, 2021 at 3:49 PM Jiri Slaby <[email protected]> wrote:
>
> On 05. 07. 21, 14:53, Zheyu Ma wrote:
> > In function 'neo_intr', the driver uses 'ch->ch_equeue' and
> > 'ch->ch_reuque'. These two pointers are initialized in 'jsm_tty_open',
> > but the interrupt handler 'neo_intr' has been registered in the probe
> > progress. If 'jsm_tty_open' has not been called at this time, it will
> > cause null pointer dereference.
> >
> > Once the driver registers the interrupt handler, the driver should be
> > ready to handle it.
> >
> > Fix this by allocating the memory at probe time and not at open time.
>
> You are allocating the buffer in jsm_tty_init now. But that is still
> called after request_irq() in probe. So care to explain how this helps
> exactly? As I understand it, you only made the window much smaller.
You are right, this may indeed still cause problems, I did not
consider this before. So maybe we should put request_irq() at the end
of the probe function.
> Anyway, I'm no expert on jsm, but AFAICS jsm_tty_open first allocates
> the buffers, brd->bd_ops->uart_init() / neo_uart_init() clears ier and
> only brd->bd_ops->param() / neo_param() enables interrupts on the device
> (by ier update and write). So how it comes an interrupt came before
> neo_param() in jsm_tty_open was called?
I considered the threat from a malicious device, which means that a
harmful peripheral may not comply with the driver's convention,
arbitrary send interrupt signals, or send malicious data. I think the
driver should also handle this situation, at least to prevent the
kernel from crashing.
Thanks,
Zheyu Ma
On Wed, Jul 7, 2021 at 10:50 AM Jiri Slaby <[email protected]> wrote:
> On 05. 07. 21, 14:53, Zheyu Ma wrote:
> So how it comes an interrupt came before
> neo_param() in jsm_tty_open was called?
If IRQ is shared we have a special debug feature to test shared IRQs
on freeing IRQ stage (*).
But it doesn't matter, the IRQ handler must survive at any stage after
the action has been listed.
*) I believe we have quite a lot of drivers that will fail that test...
--
With Best Regards,
Andy Shevchenko
On 07. 07. 21, 14:52, Andy Shevchenko wrote:
> On Wed, Jul 7, 2021 at 10:50 AM Jiri Slaby <[email protected]> wrote:
>> On 05. 07. 21, 14:53, Zheyu Ma wrote:
>
>> So how it comes an interrupt came before
>> neo_param() in jsm_tty_open was called?
>
> If IRQ is shared we have a special debug feature to test shared IRQs
> on freeing IRQ stage (*).
> But it doesn't matter, the IRQ handler must survive at any stage after
> the action has been listed.
Yes, but IRQ_NONE is returned from the ISR in that case.
The issue the patch is fixing is for a "malicious" device and I am not
sure we want to fix this -- if I can put in a malicious device, I can
use hammer to kill the box too…
thanks,
--
js
On Thu, Jul 8, 2021 at 3:13 AM Jiri Slaby <[email protected]> wrote:
>
> On 07. 07. 21, 14:52, Andy Shevchenko wrote:
> > On Wed, Jul 7, 2021 at 10:50 AM Jiri Slaby <[email protected]> wrote:
> >> On 05. 07. 21, 14:53, Zheyu Ma wrote:
> >
> >> So how it comes an interrupt came before
> >> neo_param() in jsm_tty_open was called?
> >
> > If IRQ is shared we have a special debug feature to test shared IRQs
> > on freeing IRQ stage (*).
> > But it doesn't matter, the IRQ handler must survive at any stage after
> > the action has been listed.
>
> Yes, but IRQ_NONE is returned from the ISR in that case.
>
> The issue the patch is fixing is for a "malicious" device and I am not
> sure we want to fix this -- if I can put in a malicious device, I can
> use hammer to kill the box too…
Well, this threat assumption is indeed strong, but this attack may be
real. For example, some programmable USB devices (such as FaceDancer)
may exploit vulnerabilities in the USB device driver to attack. Of
course, there has not been such an attack in the real world for PCI
devices. Or, some devices with DMA functions may also send malicious
data and some previous kernel commits have also fixed such bugs.
Anyway, thanks for your patient comments.
Regards,
Zheyu Ma
On Fri, Jul 09, 2021 at 10:00:32PM +0800, Zheyu Ma wrote:
> On Thu, Jul 8, 2021 at 3:13 AM Jiri Slaby <[email protected]> wrote:
> >
> > On 07. 07. 21, 14:52, Andy Shevchenko wrote:
> > > On Wed, Jul 7, 2021 at 10:50 AM Jiri Slaby <[email protected]> wrote:
> > >> On 05. 07. 21, 14:53, Zheyu Ma wrote:
> > >
> > >> So how it comes an interrupt came before
> > >> neo_param() in jsm_tty_open was called?
> > >
> > > If IRQ is shared we have a special debug feature to test shared IRQs
> > > on freeing IRQ stage (*).
> > > But it doesn't matter, the IRQ handler must survive at any stage after
> > > the action has been listed.
> >
> > Yes, but IRQ_NONE is returned from the ISR in that case.
> >
> > The issue the patch is fixing is for a "malicious" device and I am not
> > sure we want to fix this -- if I can put in a malicious device, I can
> > use hammer to kill the box too…
>
> Well, this threat assumption is indeed strong, but this attack may be
> real. For example, some programmable USB devices (such as FaceDancer)
> may exploit vulnerabilities in the USB device driver to attack. Of
> course, there has not been such an attack in the real world for PCI
> devices. Or, some devices with DMA functions may also send malicious
> data and some previous kernel commits have also fixed such bugs.
>
> Anyway, thanks for your patient comments.
Right now, yes, we treat USB devices as "possibly malicious". We do not
do so for PCI devices yet. If we want to do that, then we need to do a
lot of work, not just "this one call in this one driver" type of thing
as there are much bigger issues involved here.
If you wish to take on this work, as you feel PCI devices should be
treated this way, please do so! But start in the PCI core at the very
least, before worrying about the thousands of individual drivers.
good luck!
greg k-h
On Fri, Jul 9, 2021 at 10:20 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Fri, Jul 09, 2021 at 10:00:32PM +0800, Zheyu Ma wrote:
> > On Thu, Jul 8, 2021 at 3:13 AM Jiri Slaby <[email protected]> wrote:
> > >
> > > On 07. 07. 21, 14:52, Andy Shevchenko wrote:
> > > > On Wed, Jul 7, 2021 at 10:50 AM Jiri Slaby <[email protected]> wrote:
> > > >> On 05. 07. 21, 14:53, Zheyu Ma wrote:
> > > >
> > > >> So how it comes an interrupt came before
> > > >> neo_param() in jsm_tty_open was called?
> > > >
> > > > If IRQ is shared we have a special debug feature to test shared IRQs
> > > > on freeing IRQ stage (*).
> > > > But it doesn't matter, the IRQ handler must survive at any stage after
> > > > the action has been listed.
> > >
> > > Yes, but IRQ_NONE is returned from the ISR in that case.
> > >
> > > The issue the patch is fixing is for a "malicious" device and I am not
> > > sure we want to fix this -- if I can put in a malicious device, I can
> > > use hammer to kill the box too…
> >
> > Well, this threat assumption is indeed strong, but this attack may be
> > real. For example, some programmable USB devices (such as FaceDancer)
> > may exploit vulnerabilities in the USB device driver to attack. Of
> > course, there has not been such an attack in the real world for PCI
> > devices. Or, some devices with DMA functions may also send malicious
> > data and some previous kernel commits have also fixed such bugs.
> >
> > Anyway, thanks for your patient comments.
>
> Right now, yes, we treat USB devices as "possibly malicious". We do not
> do so for PCI devices yet. If we want to do that, then we need to do a
> lot of work, not just "this one call in this one driver" type of thing
> as there are much bigger issues involved here.
>
> If you wish to take on this work, as you feel PCI devices should be
> treated this way, please do so! But start in the PCI core at the very
> least, before worrying about the thousands of individual drivers.
Alright, I understand.
Thank you for your valuable suggestions, I will continue to learn and
make more contributions to the kernel.
Regards,
Zheyu Ma