diff mbox series

[SRU,Trusty,CVE-2017-18079] Input: i8042 - fix crash at boot time

Message ID 20180419130425.32065-1-kleber.souza@canonical.com
State New
Headers show
Series [SRU,Trusty,CVE-2017-18079] Input: i8042 - fix crash at boot time | expand

Commit Message

Kleber Sacilotto de Souza April 19, 2018, 1:04 p.m. UTC 
From: Chen Hong <chenhong3@huawei.com>

CVE-2017-18079

The driver checks port->exists twice in i8042_interrupt(), first when
trying to assign temporary "serio" variable, and second time when deciding
whether it should call serio_interrupt(). The value of port->exists may
change between the 2 checks, and we may end up calling serio_interrupt()
with a NULL pointer:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000050
IP: [<ffffffff8150feaf>] _spin_lock_irqsave+0x1f/0x40
PGD 0
Oops: 0002 [#1] SMP
last sysfs file:
CPU 0
Modules linked in:

Pid: 1, comm: swapper Not tainted 2.6.32-358.el6.x86_64 #1 QEMU Standard PC (i440FX + PIIX, 1996)
RIP: 0010:[<ffffffff8150feaf>]  [<ffffffff8150feaf>] _spin_lock_irqsave+0x1f/0x40
RSP: 0018:ffff880028203cc0  EFLAGS: 00010082
RAX: 0000000000010000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000282 RSI: 0000000000000098 RDI: 0000000000000050
RBP: ffff880028203cc0 R08: ffff88013e79c000 R09: ffff880028203ee0
R10: 0000000000000298 R11: 0000000000000282 R12: 0000000000000050
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000098
FS:  0000000000000000(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 0000000000000050 CR3: 0000000001a85000 CR4: 00000000001407f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 1, threadinfo ffff88013e79c000, task ffff88013e79b500)
Stack:
ffff880028203d00 ffffffff813de186 ffffffffffffff02 0000000000000000
<d> 0000000000000000 0000000000000000 0000000000000000 0000000000000098
<d> ffff880028203d70 ffffffff813e0162 ffff880028203d20 ffffffff8103b8ac
Call Trace:
<IRQ>
 [<ffffffff813de186>] serio_interrupt+0x36/0xa0
[<ffffffff813e0162>] i8042_interrupt+0x132/0x3a0
[<ffffffff8103b8ac>] ? kvm_clock_read+0x1c/0x20
[<ffffffff8103b8b9>] ? kvm_clock_get_cycles+0x9/0x10
[<ffffffff810e1640>] handle_IRQ_event+0x60/0x170
[<ffffffff8103b154>] ? kvm_guest_apic_eoi_write+0x44/0x50
[<ffffffff810e3d8e>] handle_edge_irq+0xde/0x180
[<ffffffff8100de89>] handle_irq+0x49/0xa0
[<ffffffff81516c8c>] do_IRQ+0x6c/0xf0
[<ffffffff8100b9d3>] ret_from_intr+0x0/0x11
[<ffffffff81076f63>] ? __do_softirq+0x73/0x1e0
[<ffffffff8109b75b>] ? hrtimer_interrupt+0x14b/0x260
[<ffffffff8100c1cc>] ? call_softirq+0x1c/0x30
[<ffffffff8100de05>] ? do_softirq+0x65/0xa0
[<ffffffff81076d95>] ? irq_exit+0x85/0x90
[<ffffffff81516d80>] ? smp_apic_timer_interrupt+0x70/0x9b
[<ffffffff8100bb93>] ? apic_timer_interrupt+0x13/0x20

To avoid the issue let's change the second check to test whether serio is
NULL or not.

Also, let's take i8042_lock in i8042_start() and i8042_stop() instead of
trying to be overly smart and using memory barriers.

Signed-off-by: Chen Hong <chenhong3@huawei.com>
[dtor: take lock in i8042_start()/i8042_stop()]
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
(cherry picked from commit 340d394a789518018f834ff70f7534fc463d3226)
Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
---
 drivers/input/serio/i8042.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Thadeu Lima de Souza Cascardo April 19, 2018, 1:26 p.m. UTC | #1 
Clean cherry-pick.

Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Khalid Elmously April 19, 2018, 2:50 p.m. UTC | #2 
On 2018-04-19 15:04:25 , Kleber Souza wrote:
> From: Chen Hong <chenhong3@huawei.com>
> 
> CVE-2017-18079
> 
> The driver checks port->exists twice in i8042_interrupt(), first when
> trying to assign temporary "serio" variable, and second time when deciding
> whether it should call serio_interrupt(). The value of port->exists may
> change between the 2 checks, and we may end up calling serio_interrupt()
> with a NULL pointer:
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000050
> IP: [<ffffffff8150feaf>] _spin_lock_irqsave+0x1f/0x40
> PGD 0
> Oops: 0002 [#1] SMP
> last sysfs file:
> CPU 0
> Modules linked in:
> 
> Pid: 1, comm: swapper Not tainted 2.6.32-358.el6.x86_64 #1 QEMU Standard PC (i440FX + PIIX, 1996)
> RIP: 0010:[<ffffffff8150feaf>]  [<ffffffff8150feaf>] _spin_lock_irqsave+0x1f/0x40
> RSP: 0018:ffff880028203cc0  EFLAGS: 00010082
> RAX: 0000000000010000 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: 0000000000000282 RSI: 0000000000000098 RDI: 0000000000000050
> RBP: ffff880028203cc0 R08: ffff88013e79c000 R09: ffff880028203ee0
> R10: 0000000000000298 R11: 0000000000000282 R12: 0000000000000050
> R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000098
> FS:  0000000000000000(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
> CR2: 0000000000000050 CR3: 0000000001a85000 CR4: 00000000001407f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process swapper (pid: 1, threadinfo ffff88013e79c000, task ffff88013e79b500)
> Stack:
> ffff880028203d00 ffffffff813de186 ffffffffffffff02 0000000000000000
> <d> 0000000000000000 0000000000000000 0000000000000000 0000000000000098
> <d> ffff880028203d70 ffffffff813e0162 ffff880028203d20 ffffffff8103b8ac
> Call Trace:
> <IRQ>
>  [<ffffffff813de186>] serio_interrupt+0x36/0xa0
> [<ffffffff813e0162>] i8042_interrupt+0x132/0x3a0
> [<ffffffff8103b8ac>] ? kvm_clock_read+0x1c/0x20
> [<ffffffff8103b8b9>] ? kvm_clock_get_cycles+0x9/0x10
> [<ffffffff810e1640>] handle_IRQ_event+0x60/0x170
> [<ffffffff8103b154>] ? kvm_guest_apic_eoi_write+0x44/0x50
> [<ffffffff810e3d8e>] handle_edge_irq+0xde/0x180
> [<ffffffff8100de89>] handle_irq+0x49/0xa0
> [<ffffffff81516c8c>] do_IRQ+0x6c/0xf0
> [<ffffffff8100b9d3>] ret_from_intr+0x0/0x11
> [<ffffffff81076f63>] ? __do_softirq+0x73/0x1e0
> [<ffffffff8109b75b>] ? hrtimer_interrupt+0x14b/0x260
> [<ffffffff8100c1cc>] ? call_softirq+0x1c/0x30
> [<ffffffff8100de05>] ? do_softirq+0x65/0xa0
> [<ffffffff81076d95>] ? irq_exit+0x85/0x90
> [<ffffffff81516d80>] ? smp_apic_timer_interrupt+0x70/0x9b
> [<ffffffff8100bb93>] ? apic_timer_interrupt+0x13/0x20
> 
> To avoid the issue let's change the second check to test whether serio is
> NULL or not.
> 
> Also, let's take i8042_lock in i8042_start() and i8042_stop() instead of
> trying to be overly smart and using memory barriers.
> 
> Signed-off-by: Chen Hong <chenhong3@huawei.com>
> [dtor: take lock in i8042_start()/i8042_stop()]
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> (cherry picked from commit 340d394a789518018f834ff70f7534fc463d3226)
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
> ---
>  drivers/input/serio/i8042.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index eb796fff9e62..ccf971a8629f 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -397,8 +397,10 @@ static int i8042_start(struct serio *serio)
>  {
>  	struct i8042_port *port = serio->port_data;
>  
> +	spin_lock_irq(&i8042_lock);
>  	port->exists = true;
> -	mb();
> +	spin_unlock_irq(&i8042_lock);
> +
>  	return 0;
>  }
>  
> @@ -411,16 +413,20 @@ static void i8042_stop(struct serio *serio)
>  {
>  	struct i8042_port *port = serio->port_data;
>  
> +	spin_lock_irq(&i8042_lock);
>  	port->exists = false;
> +	port->serio = NULL;
> +	spin_unlock_irq(&i8042_lock);
>  
>  	/*
> +	 * We need to make sure that interrupt handler finishes using
> +	 * our serio port before we return from this function.
>  	 * We synchronize with both AUX and KBD IRQs because there is
>  	 * a (very unlikely) chance that AUX IRQ is raised for KBD port
>  	 * and vice versa.
>  	 */
>  	synchronize_irq(I8042_AUX_IRQ);
>  	synchronize_irq(I8042_KBD_IRQ);
> -	port->serio = NULL;
>  }
>  
>  /*
> @@ -537,7 +543,7 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
>  
>  	spin_unlock_irqrestore(&i8042_lock, flags);
>  
> -	if (likely(port->exists && !filtered))
> +	if (likely(serio && !filtered))
>  		serio_interrupt(serio, data, dfl);
>  
>   out:
Acked-by: Khalid Elmously <khalid.elmously@canonical.com>
Stefan Bader April 19, 2018, 4:51 p.m. UTC | #3 
On 19.04.2018 15:04, Kleber Sacilotto de Souza wrote:
> From: Chen Hong <chenhong3@huawei.com>
> 
> CVE-2017-18079
> 
> The driver checks port->exists twice in i8042_interrupt(), first when
> trying to assign temporary "serio" variable, and second time when deciding
> whether it should call serio_interrupt(). The value of port->exists may
> change between the 2 checks, and we may end up calling serio_interrupt()
> with a NULL pointer:
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000050
> IP: [<ffffffff8150feaf>] _spin_lock_irqsave+0x1f/0x40
> PGD 0
> Oops: 0002 [#1] SMP
> last sysfs file:
> CPU 0
> Modules linked in:
> 
> Pid: 1, comm: swapper Not tainted 2.6.32-358.el6.x86_64 #1 QEMU Standard PC (i440FX + PIIX, 1996)
> RIP: 0010:[<ffffffff8150feaf>]  [<ffffffff8150feaf>] _spin_lock_irqsave+0x1f/0x40
> RSP: 0018:ffff880028203cc0  EFLAGS: 00010082
> RAX: 0000000000010000 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: 0000000000000282 RSI: 0000000000000098 RDI: 0000000000000050
> RBP: ffff880028203cc0 R08: ffff88013e79c000 R09: ffff880028203ee0
> R10: 0000000000000298 R11: 0000000000000282 R12: 0000000000000050
> R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000098
> FS:  0000000000000000(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
> CR2: 0000000000000050 CR3: 0000000001a85000 CR4: 00000000001407f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process swapper (pid: 1, threadinfo ffff88013e79c000, task ffff88013e79b500)
> Stack:
> ffff880028203d00 ffffffff813de186 ffffffffffffff02 0000000000000000
> <d> 0000000000000000 0000000000000000 0000000000000000 0000000000000098
> <d> ffff880028203d70 ffffffff813e0162 ffff880028203d20 ffffffff8103b8ac
> Call Trace:
> <IRQ>
>  [<ffffffff813de186>] serio_interrupt+0x36/0xa0
> [<ffffffff813e0162>] i8042_interrupt+0x132/0x3a0
> [<ffffffff8103b8ac>] ? kvm_clock_read+0x1c/0x20
> [<ffffffff8103b8b9>] ? kvm_clock_get_cycles+0x9/0x10
> [<ffffffff810e1640>] handle_IRQ_event+0x60/0x170
> [<ffffffff8103b154>] ? kvm_guest_apic_eoi_write+0x44/0x50
> [<ffffffff810e3d8e>] handle_edge_irq+0xde/0x180
> [<ffffffff8100de89>] handle_irq+0x49/0xa0
> [<ffffffff81516c8c>] do_IRQ+0x6c/0xf0
> [<ffffffff8100b9d3>] ret_from_intr+0x0/0x11
> [<ffffffff81076f63>] ? __do_softirq+0x73/0x1e0
> [<ffffffff8109b75b>] ? hrtimer_interrupt+0x14b/0x260
> [<ffffffff8100c1cc>] ? call_softirq+0x1c/0x30
> [<ffffffff8100de05>] ? do_softirq+0x65/0xa0
> [<ffffffff81076d95>] ? irq_exit+0x85/0x90
> [<ffffffff81516d80>] ? smp_apic_timer_interrupt+0x70/0x9b
> [<ffffffff8100bb93>] ? apic_timer_interrupt+0x13/0x20
> 
> To avoid the issue let's change the second check to test whether serio is
> NULL or not.
> 
> Also, let's take i8042_lock in i8042_start() and i8042_stop() instead of
> trying to be overly smart and using memory barriers.
> 
> Signed-off-by: Chen Hong <chenhong3@huawei.com>
> [dtor: take lock in i8042_start()/i8042_stop()]
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> (cherry picked from commit 340d394a789518018f834ff70f7534fc463d3226)
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
> ---

Applied to trusty/master-next

-Stefan

>  drivers/input/serio/i8042.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index eb796fff9e62..ccf971a8629f 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -397,8 +397,10 @@ static int i8042_start(struct serio *serio)
>  {
>  	struct i8042_port *port = serio->port_data;
>  
> +	spin_lock_irq(&i8042_lock);
>  	port->exists = true;
> -	mb();
> +	spin_unlock_irq(&i8042_lock);
> +
>  	return 0;
>  }
>  
> @@ -411,16 +413,20 @@ static void i8042_stop(struct serio *serio)
>  {
>  	struct i8042_port *port = serio->port_data;
>  
> +	spin_lock_irq(&i8042_lock);
>  	port->exists = false;
> +	port->serio = NULL;
> +	spin_unlock_irq(&i8042_lock);
>  
>  	/*
> +	 * We need to make sure that interrupt handler finishes using
> +	 * our serio port before we return from this function.
>  	 * We synchronize with both AUX and KBD IRQs because there is
>  	 * a (very unlikely) chance that AUX IRQ is raised for KBD port
>  	 * and vice versa.
>  	 */
>  	synchronize_irq(I8042_AUX_IRQ);
>  	synchronize_irq(I8042_KBD_IRQ);
> -	port->serio = NULL;
>  }
>  
>  /*
> @@ -537,7 +543,7 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
>  
>  	spin_unlock_irqrestore(&i8042_lock, flags);
>  
> -	if (likely(port->exists && !filtered))
> +	if (likely(serio && !filtered))
>  		serio_interrupt(serio, data, dfl);
>  
>   out:
>
diff mbox series

Patch

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index eb796fff9e62..ccf971a8629f 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -397,8 +397,10 @@  static int i8042_start(struct serio *serio)
 {
 	struct i8042_port *port = serio->port_data;
 
+	spin_lock_irq(&i8042_lock);
 	port->exists = true;
-	mb();
+	spin_unlock_irq(&i8042_lock);
+
 	return 0;
 }
 
@@ -411,16 +413,20 @@  static void i8042_stop(struct serio *serio)
 {
 	struct i8042_port *port = serio->port_data;
 
+	spin_lock_irq(&i8042_lock);
 	port->exists = false;
+	port->serio = NULL;
+	spin_unlock_irq(&i8042_lock);
 
 	/*
+	 * We need to make sure that interrupt handler finishes using
+	 * our serio port before we return from this function.
 	 * We synchronize with both AUX and KBD IRQs because there is
 	 * a (very unlikely) chance that AUX IRQ is raised for KBD port
 	 * and vice versa.
 	 */
 	synchronize_irq(I8042_AUX_IRQ);
 	synchronize_irq(I8042_KBD_IRQ);
-	port->serio = NULL;
 }
 
 /*
@@ -537,7 +543,7 @@  static irqreturn_t i8042_interrupt(int irq, void *dev_id)
 
 	spin_unlock_irqrestore(&i8042_lock, flags);
 
-	if (likely(port->exists && !filtered))
+	if (likely(serio && !filtered))
 		serio_interrupt(serio, data, dfl);
 
  out: