linux-kernel - Re: [PATCH] input: i8042 - fix a double-fetch vulnerability introduced by GCC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y2THdZRV7Wg/1mSC@debian.me>
Date:   Fri, 4 Nov 2022 15:04:05 +0700
From:   Bagas Sanjaya <bagasdotme@...il.com>
To:     Kunbo Zhang <absoler@...il.nju.edu.cn>
Cc:     dmitry.torokhov@...il.com, tiwai@...e.de,
        wsa+renesas@...g-engineering.com, linux-kernel@...r.kernel.org,
        linux-input@...r.kernel.org, security@...nel.org
Subject: Re: [PATCH] input: i8042 - fix a double-fetch vulnerability
 introduced by GCC

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@...il.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@...r.kernel.org to the SoB area.

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)