All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] tty: serial: jsm: allocate queue buffer at probe time
@ 2021-07-05 12:53 Zheyu Ma
  2021-07-07  7:49 ` Jiri Slaby
       [not found] ` <CAHp75Vdne2fVAdmMYPn71T8LnSNMxVhBVK8dbmMASTSTUnOjBA@mail.gmail.com>
  0 siblings, 2 replies; 9+ messages in thread
From: Zheyu Ma @ 2021-07-05 12:53 UTC (permalink / raw)
  To: gregkh, jirislaby; +Cc: zheyuma97, linux-serial, linux-kernel

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 <zheyuma97@gmail.com>
---
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


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

* Re: [PATCH v4] tty: serial: jsm: allocate queue buffer at probe time
  2021-07-05 12:53 [PATCH v4] tty: serial: jsm: allocate queue buffer at probe time Zheyu Ma
@ 2021-07-07  7:49 ` Jiri Slaby
  2021-07-07 12:28   ` Zheyu Ma
  2021-07-07 12:52   ` Andy Shevchenko
       [not found] ` <CAHp75Vdne2fVAdmMYPn71T8LnSNMxVhBVK8dbmMASTSTUnOjBA@mail.gmail.com>
  1 sibling, 2 replies; 9+ messages in thread
From: Jiri Slaby @ 2021-07-07  7:49 UTC (permalink / raw)
  To: Zheyu Ma, gregkh; +Cc: linux-serial, linux-kernel

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

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

* Re: [PATCH v4] tty: serial: jsm: allocate queue buffer at probe time
       [not found] ` <CAHp75Vdne2fVAdmMYPn71T8LnSNMxVhBVK8dbmMASTSTUnOjBA@mail.gmail.com>
@ 2021-07-07 10:37   ` Zheyu Ma
  0 siblings, 0 replies; 9+ messages in thread
From: Zheyu Ma @ 2021-07-07 10:37 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: gregkh, jirislaby, linux-serial, linux-kernel

On Wed, Jul 7, 2021 at 4:14 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
>
>
> On Monday, July 5, 2021, Zheyu Ma <zheyuma97@gmail.com> 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

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

* Re: [PATCH v4] tty: serial: jsm: allocate queue buffer at probe time
  2021-07-07  7:49 ` Jiri Slaby
@ 2021-07-07 12:28   ` Zheyu Ma
  2021-07-07 12:52   ` Andy Shevchenko
  1 sibling, 0 replies; 9+ messages in thread
From: Zheyu Ma @ 2021-07-07 12:28 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: Greg KH, linux-serial, Linux Kernel Mailing List

On Wed, Jul 7, 2021 at 3:49 PM Jiri Slaby <jirislaby@kernel.org> 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

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

* Re: [PATCH v4] tty: serial: jsm: allocate queue buffer at probe time
  2021-07-07  7:49 ` Jiri Slaby
  2021-07-07 12:28   ` Zheyu Ma
@ 2021-07-07 12:52   ` Andy Shevchenko
  2021-07-07 19:13     ` Jiri Slaby
  1 sibling, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-07-07 12:52 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: Zheyu Ma, Greg Kroah-Hartman, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List

On Wed, Jul 7, 2021 at 10:50 AM Jiri Slaby <jirislaby@kernel.org> 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

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

* Re: [PATCH v4] tty: serial: jsm: allocate queue buffer at probe time
  2021-07-07 12:52   ` Andy Shevchenko
@ 2021-07-07 19:13     ` Jiri Slaby
  2021-07-09 14:00       ` Zheyu Ma
  0 siblings, 1 reply; 9+ messages in thread
From: Jiri Slaby @ 2021-07-07 19:13 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Zheyu Ma, Greg Kroah-Hartman, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List

On 07. 07. 21, 14:52, Andy Shevchenko wrote:
> On Wed, Jul 7, 2021 at 10:50 AM Jiri Slaby <jirislaby@kernel.org> 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

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

* Re: [PATCH v4] tty: serial: jsm: allocate queue buffer at probe time
  2021-07-07 19:13     ` Jiri Slaby
@ 2021-07-09 14:00       ` Zheyu Ma
  2021-07-09 14:20         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 9+ messages in thread
From: Zheyu Ma @ 2021-07-09 14:00 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: Andy Shevchenko, Greg Kroah-Hartman, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List

On Thu, Jul 8, 2021 at 3:13 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 07. 07. 21, 14:52, Andy Shevchenko wrote:
> > On Wed, Jul 7, 2021 at 10:50 AM Jiri Slaby <jirislaby@kernel.org> 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

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

* Re: [PATCH v4] tty: serial: jsm: allocate queue buffer at probe time
  2021-07-09 14:00       ` Zheyu Ma
@ 2021-07-09 14:20         ` Greg Kroah-Hartman
  2021-07-09 14:55           ` Zheyu Ma
  0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2021-07-09 14:20 UTC (permalink / raw)
  To: Zheyu Ma
  Cc: Jiri Slaby, Andy Shevchenko, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List

On Fri, Jul 09, 2021 at 10:00:32PM +0800, Zheyu Ma wrote:
> On Thu, Jul 8, 2021 at 3:13 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> >
> > On 07. 07. 21, 14:52, Andy Shevchenko wrote:
> > > On Wed, Jul 7, 2021 at 10:50 AM Jiri Slaby <jirislaby@kernel.org> 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

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

* Re: [PATCH v4] tty: serial: jsm: allocate queue buffer at probe time
  2021-07-09 14:20         ` Greg Kroah-Hartman
@ 2021-07-09 14:55           ` Zheyu Ma
  0 siblings, 0 replies; 9+ messages in thread
From: Zheyu Ma @ 2021-07-09 14:55 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Andy Shevchenko, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List

On Fri, Jul 9, 2021 at 10:20 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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 <jirislaby@kernel.org> wrote:
> > >
> > > On 07. 07. 21, 14:52, Andy Shevchenko wrote:
> > > > On Wed, Jul 7, 2021 at 10:50 AM Jiri Slaby <jirislaby@kernel.org> 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

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

end of thread, other threads:[~2021-07-09 14:55 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-05 12:53 [PATCH v4] tty: serial: jsm: allocate queue buffer at probe time Zheyu Ma
2021-07-07  7:49 ` Jiri Slaby
2021-07-07 12:28   ` Zheyu Ma
2021-07-07 12:52   ` Andy Shevchenko
2021-07-07 19:13     ` Jiri Slaby
2021-07-09 14:00       ` Zheyu Ma
2021-07-09 14:20         ` Greg Kroah-Hartman
2021-07-09 14:55           ` Zheyu Ma
     [not found] ` <CAHp75Vdne2fVAdmMYPn71T8LnSNMxVhBVK8dbmMASTSTUnOjBA@mail.gmail.com>
2021-07-07 10:37   ` Zheyu Ma

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.