diff mbox series

input: i8042 - fix a double-fetch vulnerability introduced by GCC

Message ID 20221104072347.74314-1-absoler@smail.nju.edu.cn
State New
Headers
Series input: i8042 - fix a double-fetch vulnerability introduced by GCC |

Commit Message

Kunbo Zhang Nov. 4, 2022, 7:23 a.m. UTC 
  We found GCC (at least 9.4.0 and 12.1) introduces a double-fetch of `i8042_ports[I8042_AUX_PORT_NO].serio` at drivers/input/serio/i8042.c:408.

One comparison of the global variable `i8042_ports[I8042_AUX_PORT_NO].serio` has been compiled to three ones,
and thus two extra fetches are introduced.
As in the source code, the global variable is tested (at line 408) before three assignments of irq_bit, disable_bit and port_name.
However, as shown in the following disassembly of i8042_port_close(), 
the variable (0x0(%rip)) is fetched and tested three times for each 
assignment of irq_bit, disable_bit and port_name.

0000000000000e50 <i8042_port_close>:
i8042_port_close():
./drivers/input/serio/i8042.c:408
     e50:	48 39 3d 00 00 00 00    cmp    %rdi,0x0(%rip)        # first load
./drivers/input/serio/i8042.c:403
     e57:	41 54                   push   %r12
./drivers/input/serio/i8042.c:408
     e59:	b8 ef ff ff ff          mov    $0xffffffef,%eax
     e5e:	49 c7 c4 00 00 00 00    mov    $0x0,%r12
./drivers/input/serio/i8042.c:403
     e65:	55                      push   %rbp
./drivers/input/serio/i8042.c:408
     e66:	48 c7 c2 00 00 00 00    mov    $0x0,%rdx
./drivers/input/serio/i8042.c:419
     e6d:	be 60 10 00 00          mov    $0x1060,%esi
./drivers/input/serio/i8042.c:403
     e72:	53                      push   %rbx
./drivers/input/serio/i8042.c:408
     e73:	bb df ff ff ff          mov    $0xffffffdf,%ebx
     e78:	0f 45 d8                cmovne %eax,%ebx
     e7b:	0f 95 c0                setne  %al
     e7e:	83 e8 03                sub    $0x3,%eax
     e81:	48 39 3d 00 00 00 00    cmp    %rdi,0x0(%rip)        # second load
     e88:	40 0f 94 c5             sete   %bpl
     e8c:	83 c5 01                add    $0x1,%ebp
     e8f:	48 39 3d 00 00 00 00    cmp    %rdi,0x0(%rip)        # third load
./drivers/input/serio/i8042.c:419
     e96:	48 c7 c7 00 00 00 00    mov    $0x0,%rdi
./drivers/input/serio/i8042.c:408
     e9d:	4c 0f 45 e2             cmovne %rdx,%r12

We have not found any lock protection for the three fetches of `i8042_ports[I8042_AUX_PORT_NO].serio` yet.
If the value of this global variable is modified concurrently among the three fetches, the corresponding assignment of 
disable_bit or port_name will possibly be incorrect.

This patch fixs this by saving the checked value in advance and using a barrier() to prevent compiler optimizations.
This is inspired by a similar compiler-introduced double fetch situation [1] in driver/xen (?).

[1] GitHub link of commit <8135cf8b092723dbfcc611fe6fdcb3a36c9951c5> ( Save xen_pci_op commands before processing it )
---
 drivers/input/serio/i8042.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


Signed-off-by: Kunbo Zhang <absoler@smail.nju.edu.cn>

Comments

Bagas Sanjaya Nov. 4, 2022, 8:04 a.m. UTC | #1 
On Fri, Nov 04, 2022 at 03:23:47PM +0800, Kunbo Zhang wrote:
> We found GCC (at least 9.4.0 and 12.1) introduces a double-fetch of `i8042_ports[I8042_AUX_PORT_NO].serio` at drivers/input/serio/i8042.c:408.
> 
> One comparison of the global variable `i8042_ports[I8042_AUX_PORT_NO].serio` has been compiled to three ones,
> and thus two extra fetches are introduced.
> As in the source code, the global variable is tested (at line 408) before three assignments of irq_bit, disable_bit and port_name.
> However, as shown in the following disassembly of i8042_port_close(), 
> the variable (0x0(%rip)) is fetched and tested three times for each 
> assignment of irq_bit, disable_bit and port_name.
> 
> 0000000000000e50 <i8042_port_close>:
> i8042_port_close():
> ./drivers/input/serio/i8042.c:408
>      e50:	48 39 3d 00 00 00 00    cmp    %rdi,0x0(%rip)        # first load
> ./drivers/input/serio/i8042.c:403
>      e57:	41 54                   push   %r12
> ./drivers/input/serio/i8042.c:408
>      e59:	b8 ef ff ff ff          mov    $0xffffffef,%eax
>      e5e:	49 c7 c4 00 00 00 00    mov    $0x0,%r12
> ./drivers/input/serio/i8042.c:403
>      e65:	55                      push   %rbp
> ./drivers/input/serio/i8042.c:408
>      e66:	48 c7 c2 00 00 00 00    mov    $0x0,%rdx
> ./drivers/input/serio/i8042.c:419
>      e6d:	be 60 10 00 00          mov    $0x1060,%esi
> ./drivers/input/serio/i8042.c:403
>      e72:	53                      push   %rbx
> ./drivers/input/serio/i8042.c:408
>      e73:	bb df ff ff ff          mov    $0xffffffdf,%ebx
>      e78:	0f 45 d8                cmovne %eax,%ebx
>      e7b:	0f 95 c0                setne  %al
>      e7e:	83 e8 03                sub    $0x3,%eax
>      e81:	48 39 3d 00 00 00 00    cmp    %rdi,0x0(%rip)        # second load
>      e88:	40 0f 94 c5             sete   %bpl
>      e8c:	83 c5 01                add    $0x1,%ebp
>      e8f:	48 39 3d 00 00 00 00    cmp    %rdi,0x0(%rip)        # third load
> ./drivers/input/serio/i8042.c:419
>      e96:	48 c7 c7 00 00 00 00    mov    $0x0,%rdi
> ./drivers/input/serio/i8042.c:408
>      e9d:	4c 0f 45 e2             cmovne %rdx,%r12
> 
> We have not found any lock protection for the three fetches of `i8042_ports[I8042_AUX_PORT_NO].serio` yet.
> If the value of this global variable is modified concurrently among the three fetches, the corresponding assignment of 
> disable_bit or port_name will possibly be incorrect.
> 
> This patch fixs this by saving the checked value in advance and using a barrier() to prevent compiler optimizations.
> This is inspired by a similar compiler-introduced double fetch situation [1] in driver/xen (?).
> 
> [1] GitHub link of commit <8135cf8b092723dbfcc611fe6fdcb3a36c9951c5> ( Save xen_pci_op commands before processing it )
> ---
>  drivers/input/serio/i8042.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index f9486495baef..554a2340ca84 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -405,7 +405,9 @@ static void i8042_port_close(struct serio *serio)
>  	int disable_bit;
>  	const char *port_name;
>  
> -	if (serio == i8042_ports[I8042_AUX_PORT_NO].serio) {
> +	struct serio *tmp = i8042_ports[I8042_AUX_PORT_NO].serio;
> +	barrier();
> +	if (serio == tmp) {
>  		irq_bit = I8042_CTR_AUXINT;
>  		disable_bit = I8042_CTR_AUXDIS;
>  		port_name = "AUX";
> 
> Signed-off-by: Kunbo Zhang <absoler@smail.nju.edu.cn>
> 

Regarding patch description, there are several guides:

  * Wrap the text (excluding code or terminal output) at 80 character
    or less.
  * Please write in imperative mood instead (no first-person pronouns [I, we],
    "make foo do bar").
  * When referring to past commits, the format should be
    --pretty=format:'%h ("%s")'. The commit hash is at least 12
    characters long (set core.abbrev to 12 on your .gitconfig)
  * Last but not least, place your SoB at the end of description (before
    three dash line).

Also, is this patch also applies to stable branches? If so, add Cc:
stable@vger.kernel.org to the SoB area.

Thanks.
Greg KH Nov. 4, 2022, 10:45 a.m. UTC | #2 
On Fri, Nov 04, 2022 at 03:23:47PM +0800, Kunbo Zhang wrote:
> We found GCC (at least 9.4.0 and 12.1) introduces a double-fetch of `i8042_ports[I8042_AUX_PORT_NO].serio` at drivers/input/serio/i8042.c:408.
> 
> One comparison of the global variable `i8042_ports[I8042_AUX_PORT_NO].serio` has been compiled to three ones,
> and thus two extra fetches are introduced.

And what problem does this cause?

> As in the source code, the global variable is tested (at line 408) before three assignments of irq_bit, disable_bit and port_name.
> However, as shown in the following disassembly of i8042_port_close(), 
> the variable (0x0(%rip)) is fetched and tested three times for each 
> assignment of irq_bit, disable_bit and port_name.

There should not be any problem with this as that value does not ever
change except in rare cases (shutdown or init).

> 
> 0000000000000e50 <i8042_port_close>:
> i8042_port_close():
> ./drivers/input/serio/i8042.c:408
>      e50:	48 39 3d 00 00 00 00    cmp    %rdi,0x0(%rip)        # first load
> ./drivers/input/serio/i8042.c:403
>      e57:	41 54                   push   %r12
> ./drivers/input/serio/i8042.c:408
>      e59:	b8 ef ff ff ff          mov    $0xffffffef,%eax
>      e5e:	49 c7 c4 00 00 00 00    mov    $0x0,%r12
> ./drivers/input/serio/i8042.c:403
>      e65:	55                      push   %rbp
> ./drivers/input/serio/i8042.c:408
>      e66:	48 c7 c2 00 00 00 00    mov    $0x0,%rdx
> ./drivers/input/serio/i8042.c:419
>      e6d:	be 60 10 00 00          mov    $0x1060,%esi
> ./drivers/input/serio/i8042.c:403
>      e72:	53                      push   %rbx
> ./drivers/input/serio/i8042.c:408
>      e73:	bb df ff ff ff          mov    $0xffffffdf,%ebx
>      e78:	0f 45 d8                cmovne %eax,%ebx
>      e7b:	0f 95 c0                setne  %al
>      e7e:	83 e8 03                sub    $0x3,%eax
>      e81:	48 39 3d 00 00 00 00    cmp    %rdi,0x0(%rip)        # second load
>      e88:	40 0f 94 c5             sete   %bpl
>      e8c:	83 c5 01                add    $0x1,%ebp
>      e8f:	48 39 3d 00 00 00 00    cmp    %rdi,0x0(%rip)        # third load
> ./drivers/input/serio/i8042.c:419
>      e96:	48 c7 c7 00 00 00 00    mov    $0x0,%rdi
> ./drivers/input/serio/i8042.c:408
>      e9d:	4c 0f 45 e2             cmovne %rdx,%r12
> 
> We have not found any lock protection for the three fetches of `i8042_ports[I8042_AUX_PORT_NO].serio` yet.
> If the value of this global variable is modified concurrently among the three fetches, the corresponding assignment of 
> disable_bit or port_name will possibly be incorrect.

When can that modification happen?

And if you really want to protect it, use the existing lock in the
structure, don't manually attempt to place calls to barrier(), that will
not work, sorry.

thanks,

greg k-h
Dmitry Torokhov Nov. 4, 2022, 6:23 p.m. UTC | #3 
Hi Greg,

On Fri, Nov 04, 2022 at 11:45:48AM +0100, Greg KH wrote:
> On Fri, Nov 04, 2022 at 03:23:47PM +0800, Kunbo Zhang wrote:
> > As in the source code, the global variable is tested (at line 408) before three assignments of irq_bit, disable_bit and port_name.
> > However, as shown in the following disassembly of i8042_port_close(), 
> > the variable (0x0(%rip)) is fetched and tested three times for each 
> > assignment of irq_bit, disable_bit and port_name.
> 
> There should not be any problem with this as that value does not ever
> change except in rare cases (shutdown or init).

We use this chunk only to establish identity of the port, we do not
expect instances to change while driver operates, so I do not think
there is any concern with re-fetching/re-checking the port while it is
being closed.

Thanks.
diff mbox series

Patch

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index f9486495baef..554a2340ca84 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -405,7 +405,9 @@  static void i8042_port_close(struct serio *serio)
 	int disable_bit;
 	const char *port_name;
 
-	if (serio == i8042_ports[I8042_AUX_PORT_NO].serio) {
+	struct serio *tmp = i8042_ports[I8042_AUX_PORT_NO].serio;
+	barrier();
+	if (serio == tmp) {
 		irq_bit = I8042_CTR_AUXINT;
 		disable_bit = I8042_CTR_AUXDIS;
 		port_name = "AUX";