Message ID | 20221104072347.74314-1-absoler@smail.nju.edu.cn |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp227647wru; Fri, 4 Nov 2022 00:29:11 -0700 (PDT) X-Google-Smtp-Source: AMsMyM54nriYnEK8/abxUdQr4TDaC6NVKA8Xc0qjdiUZjA2QwwlJ2tVT7M04RxRsILIMCfhLngR2 X-Received: by 2002:a63:ff55:0:b0:438:fa5d:af36 with SMTP id s21-20020a63ff55000000b00438fa5daf36mr29492418pgk.533.1667546951337; Fri, 04 Nov 2022 00:29:11 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667546951; cv=none; d=google.com; s=arc-20160816; b=Xjw45C+h5uID7/7iKBqK3ibR/Il37fEXfcYxCNF0vIapHGGVRxWy0Hpa4nNeGL9MUC QS1KwLX+tOFKAa14BNU8tzqy/nAPYlwMgwJYhNtuS0TKLwBRMsrW5vgY9WawZ1K6SlB4 rX1j5Ka3kS6BSanYAV6NeMwPXbrQDaJZxxwapo1yppPiK0TWJHwNutfJyZcImVzA6UnM thWWZ0O3Gt5sbhlzcMrknjcWgxNTs30C20a9rADuvkSurxNYrNFAYgjS4YzcTnIIyad+ 1OHsuKp6kS9HHO9/Au4vg6KQLtzTXjfa6B+GhvO2x1XaiHM/9gqJoHKI2aG0WZxdcSET VL1A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:feedback-id:content-transfer-encoding :mime-version:message-id:date:subject:cc:to:from; bh=I1h8mzuDtZJQhyLLuYwgbPdLjkYw2em/Y6f3GQqT5Wo=; b=prv38LuHhFcN6DYbcCWJhn5lwadKp+7y8POoHfrDY0SulNAYt+MYdj8rg5n0E7e9WZ OhSsw9OFzlsGFlTo6h3i0DibWuRSk+qQlfUE4Gpk+xDDOFJBPlWG5H9rzA3gctFW2o58 V/aN8wqu2RF/HOjgjX0lMo2zKbWwJub8fl1pvyzjU/B2e3sGHxM6rvYPdOVBk5HRrfEe Kq5ifWfe+yzWgWBepv9Xc/n/qOw3/M3fqH1XWRArN0GwQGBMg7PYEWDnjufajldP74r6 r3k/w9u2sheuZ92TtbPhlWXNWJea/jWiADWTajL6R9+MxpbbMvNQ816LOTaC4MjNJT7R XlfA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=nju.edu.cn Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id l7-20020a635707000000b0043aa67c7d8esi3874793pgb.738.2022.11.04.00.28.58; Fri, 04 Nov 2022 00:29:11 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=nju.edu.cn Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231324AbiKDH0S (ORCPT <rfc822;jimliu8233@gmail.com> + 99 others); Fri, 4 Nov 2022 03:26:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53090 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229779AbiKDH0Q (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 4 Nov 2022 03:26:16 -0400 Received: from smtpbgeu2.qq.com (smtpbgeu2.qq.com [18.194.254.142]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2176518363; Fri, 4 Nov 2022 00:26:08 -0700 (PDT) X-QQ-mid: bizesmtp75t1667546649tv9fay8q Received: from localhost.localdomain ( [58.213.8.169]) by bizesmtp.qq.com (ESMTP) with id ; Fri, 04 Nov 2022 15:24:08 +0800 (CST) X-QQ-SSF: 0140000000000030B000000A0000000 X-QQ-FEAT: FVl8EHhfVR5sZRNRY9/DeJL8TZ/JzmaTzPbfcQeUkmkwgHG3TgzG7YguMkqwy yaQfw+EnjutXfftW/VCbeBl/TfaZCKBW2+Tai+grdWNRQ7bVMqRo8eL5hj9XDxKwdnWVJa4 3xp1I/AsLqrRDSXgrca14LAlBItPg97/+NoX/VhKn3jX2MiM0qBumuYrJx8lkRKC4kNRKY9 PfgdnibQfjejhJ+eZZQkH71Qc4KY5e0m0y9gvNbDdeLcu9lW97h4P1sz051DZacDRatllrx bJWCDDK7cMDQ6rmSheAYzwzwahFpX4UL85BoK2x8Mni1J9E3ASq/Va5D7FUW42JeNQ/O/MS zEFNu67stA7otbGcPr/cJY/mlSuvovGkzZlA2pvnM944vlf/Okkl2i7p57MPTaKMyRWQR9M X-QQ-GoodBg: 1 From: Kunbo Zhang <absoler@smail.nju.edu.cn> To: dmitry.torokhov@gmail.com, tiwai@suse.de, wsa+renesas@sang-engineering.com Cc: linux-kernel@vger.kernel.org, linux-input@vger.kernel.org, security@kernel.org, Kunbo Zhang <absoler@smail.nju.edu.cn> Subject: [PATCH] input: i8042 - fix a double-fetch vulnerability introduced by GCC Date: Fri, 4 Nov 2022 15:23:47 +0800 Message-Id: <20221104072347.74314-1-absoler@smail.nju.edu.cn> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-QQ-SENDSIZE: 520 Feedback-ID: bizesmtp:smail.nju.edu.cn:qybglogicsvr:qybglogicsvr6 X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE, SPF_PASS,T_SPF_HELO_TEMPERROR autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748549712177172953?= X-GMAIL-MSGID: =?utf-8?q?1748549712177172953?= |
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
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.
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
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 --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";