Received-SPF: pass (google.com: domain of linux-kernel+bounces-209351-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Zhuo, Qiuxu" <qiuxu.zhuo@intel.com>
To: Kees Cook <kees@kernel.org>, Thomas Gleixner <tglx@linutronix.de>
CC: David Gow <davidgow@google.com>, Ingo Molnar <mingo@redhat.com>, "Borislav
 Petkov" <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>,
	"x86@kernel.org" <x86@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>, "Sean
 Christopherson" <seanjc@google.com>, Peter Zijlstra <peterz@infradead.org>,
	Arnd Bergmann <arnd@arndb.de>, "Kirill A. Shutemov"
	<kirill.shutemov@linux.intel.com>, Nadav Amit <nadav.amit@gmail.com>,
	Masahiro Yamada <masahiroy@kernel.org>, Christian Brauner
	<brauner@kernel.org>, David Howells <dhowells@redhat.com>, Uros Bizjak
	<ubizjak@gmail.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "linux-hardening@vger.kernel.org"
	<linux-hardening@vger.kernel.org>
Subject: RE: [PATCH] x86/uaccess: Fix missed zeroing of ia32 u64 get_user()
 range checking
Thread-Topic: [PATCH] x86/uaccess: Fix missed zeroing of ia32 u64 get_user()
 range checking
Thread-Index: AQHau3mO4jAkhXdPBUmV7VcXJhmY/LHCHgQw
Date: Tue, 11 Jun 2024 06:57:04 +0000
Message-ID: <CY8PR11MB71349DFC7229D5208824CAFB89C72@CY8PR11MB7134.namprd11.prod.outlook.com>
References: <20240610210213.work.143-kees@kernel.org>
In-Reply-To: <20240610210213.work.143-kees@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?KuM6eBV1Kfg/G03gRNcWP5AMFNGGzyFQsNkY7Mm5UkOFCjqt+ZQAWnVoq0xC?=
 =?us-ascii?Q?fiu7tWC/LwInTBV6WIU+zWx/K7U4kY9h7y+8C6MoRTHHSxXt8XmoJdNy59HZ?=
 =?us-ascii?Q?qbwIOzHawasTFnWBqyM6YELQvEMDiISIh9hEMUCZQ/kHPRgaIDCSwnDQRCtU?=
 =?us-ascii?Q?WNyNjtFYKumzMoFf/H3oL+5H+F/T8xAZUKzpT/TJmFvwrIs4WsGKjAIhN02P?=
 =?us-ascii?Q?FcHiPb7HykJZDlP896CAa/HtlApMxjOY61/xa8ftch0UqodJKeC17LigSrqc?=
 =?us-ascii?Q?umLrOfb7jTC+D8Q3wwCf0/m/YxlX8aHusLhaCK3IVQj1WukASLT+Ja66ITCB?=
 =?us-ascii?Q?cM2+dy4tdt/Hgxjw7HhOtp8/pa1EtQ/JPDx5Srpt6lGkgvQpHP5vaQhvCdnM?=
 =?us-ascii?Q?ecQuPCAiYrhyS2IVE9qpMth4mwv1F6juLW9+EZUYQeWbyk9CufVq4dRpysYp?=
 =?us-ascii?Q?agF+K9vTFUXuhbaYRQZtFDUxQ1tDK8+D6LSYQR1KzimO0HCjt2R1UNMlzwM/?=
 =?us-ascii?Q?QsPcm7rfAt0C3lPixK9BlrTST6lOGfZx5JGNfQs25nRMqzZWQ55zuT4eom2H?=
 =?us-ascii?Q?b3OEmuryFcF7QgnYYdHPfHlYgl/ek6dWYTwNYTgJP0cWjTCaPVLK5Ei6y8ea?=
 =?us-ascii?Q?P6mvamu3C2xqdxs2gIA4NzRFeD/2lENGUcgRCh1nfyVIkDZK8HOix7jKhGPO?=
 =?us-ascii?Q?JtLqbt10bjIBc4ED6iN2m3DiFhMWEvqOVkat6E5FVn/AhdqLgMTMhzoZM/SV?=
 =?us-ascii?Q?vlqhYgNNp1M7jnCsC++2ACH1NbKQiM6FryoraZhx2mOaLSf25V9J9t4IitmB?=
 =?us-ascii?Q?hGhOrt7/IjPDGfIKbm5WnZKPGgCQEwh9EbDAZ2Y7EnqXg4OnR0wMYjZobHmf?=
 =?us-ascii?Q?mvVOudYdPyNs/fiH+qfzwYOJpRGjHMf47lJxp/rSDGGBxucqCidS5V28uymW?=
 =?us-ascii?Q?Qc9KUbPKxtz6b+oiZK+n3pSqy275IC86sj5dYxcF1uDRhTPUeQne4ALPrZVp?=
 =?us-ascii?Q?WbZYZi+DNb1RVia0mp68T2NfR3/hrVxsxN9PL6oyAkVFIaA+Vh9uaxOA1SUp?=
 =?us-ascii?Q?JiLzuPxFvXqji/J3MJY8LUcA8isPdqqGSTgINSH5YK7v9rRybtWsymUNKyVO?=
 =?us-ascii?Q?bGgcrPuanE7jYtIw+cES9eMc1BKlJdqyXg8tzOEGkcEIdHCFT7tI7HkXJKBh?=
 =?us-ascii?Q?wNyEHhuck72xRAEhROR7NkR2Ay99pxg5uubl5PtOesHCNx9tN9hbFZ51znSw?=
 =?us-ascii?Q?rmqEhILg6f1XnW538ndpZ1FQv6WS8WeQIghK0sYKGFTjAOSGdKLN60uCvfgN?=
 =?us-ascii?Q?EmPZIxdUxxOVii+YsumzjMYaqhhND6pebq2ZrSCc+wRBSaPJGBksSpgyoHR8?=
 =?us-ascii?Q?NY+03gV5hAZau5yf2yLm4Xnal6YHSO21qr1LMHo3A2qmkg24wYaLlUgo1tqm?=
 =?us-ascii?Q?id5hHYldw4rq6xdIkG7327CGh189Xr8HtTOZH9G9bmn4nlJo4W63elHX+Nsi?=
 =?us-ascii?Q?HtKcNtZ+RA/NoFQffV+8EttawiLDnC3oAQzEOGEnfOv9QCtPImVM9lAfKG3v?=
 =?us-ascii?Q?sGlM29VfRjuxWD7qTkRQwD+X+CGHdhpgKO/80IXV?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CY8PR11MB7134.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0261d8aa-559d-4142-7b49-08dc89e3b3ae
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Jun 2024 06:57:04.4650
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: TQhmMbYqUyMikyN4d3/egoylITLzVWX4s//m+f5SQeLJG5NFAgSRf4skEboGhCM63VcdncvzN2sCnavEFgokMw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4633
X-OriginatorOrg: intel.com

> From: Kees Cook <kees@kernel.org>
> [...]
> Subject: [PATCH] x86/uaccess: Fix missed zeroing of ia32 u64 get_user() r=
ange
> checking
>=20
> When reworking the range checking for get_user(), the get_user_8() case o=
n
> 32-bit wasn't zeroing the high register. (The jump to bad_get_user_8 was
> accidentally dropped.) Restore the correct error handling destination (an=
d
> rename the jump to using the expected ".L" prefix).
>=20
> While here, switch to using a named argument ("size") for the call templa=
te
> ("%c4" to "%c[size]") as already used in the other call templates in this=
 file.
>=20
> Found after moving the usercopy selftests to KUnit:
>=20
>       # usercopy_test_invalid: EXPECTATION FAILED at
>       lib/usercopy_kunit.c:278
>       Expected val_u64 =3D=3D 0, but
>           val_u64 =3D=3D -60129542144 (0xfffffff200000000)
>=20
> Reported-by: David Gow <davidgow@google.com>
> Closes: https://lore.kernel.org/all/CABVgOSn=3Dtb=3DLj9SxHuT4_9MTjjKVxsq-
> ikdXC4kGHO4CfKVmGQ@mail.gmail.com
> Fixes: b19b74bc99b1 ("x86/mm: Rework address range check in get_user()
> and put_user()")
> Signed-off-by: Kees Cook <kees@kernel.org>
> [...]
>  arch/x86/include/asm/uaccess.h | 4 ++--
>  arch/x86/lib/getuser.S         | 6 +++++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
>=20
> diff --git a/arch/x86/include/asm/uaccess.h
> b/arch/x86/include/asm/uaccess.h index 0f9bab92a43d..3a7755c1a441
> 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -78,10 +78,10 @@ extern int __get_user_bad(void);
>  	int __ret_gu;							\
>  	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
>  	__chk_user_ptr(ptr);						\
> -	asm volatile("call __" #fn "_%c4"				\
> +	asm volatile("call __" #fn "_%c[size]"				\
>  		     : "=3Da" (__ret_gu), "=3Dr" (__val_gu),		\
>  			ASM_CALL_CONSTRAINT
> 	\
> -		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
> +		     : "0" (ptr), [size] "i" (sizeof(*(ptr))));		\
>  	instrument_get_user(__val_gu);
> 	\
>  	(x) =3D (__force __typeof__(*(ptr))) __val_gu;			\
>  	__builtin_expect(__ret_gu, 0);					\
> diff --git a/arch/x86/lib/getuser.S b/arch/x86/lib/getuser.S index
> 10d5ed8b5990..a1cb3a4e6742 100644
> --- a/arch/x86/lib/getuser.S
> +++ b/arch/x86/lib/getuser.S
> @@ -44,7 +44,11 @@
>  	or %rdx, %rax
>  .else
>  	cmp $TASK_SIZE_MAX-\size+1, %eax
> +.if \size !=3D 8
>  	jae .Lbad_get_user
> +.else
> +	jae .Lbad_get_user_8
> +.endif
>  	sbb %edx, %edx		/* array_index_mask_nospec() */
>  	and %edx, %eax
>  .endif
> @@ -154,7 +158,7 @@ SYM_CODE_END(__get_user_handle_exception)
>  #ifdef CONFIG_X86_32
>  SYM_CODE_START_LOCAL(__get_user_8_handle_exception)
>  	ASM_CLAC
> -bad_get_user_8:
> +.Lbad_get_user_8:
>  	xor %edx,%edx
>  	xor %ecx,%ecx
>  	mov $(-EFAULT),%_ASM_AX
> --
> 2.34.1

LGTM. Thanks!

Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>