From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EE532C4332F for ; Wed, 9 Nov 2022 11:25:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230493AbiKILZT (ORCPT ); Wed, 9 Nov 2022 06:25:19 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43108 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231219AbiKILZJ (ORCPT ); Wed, 9 Nov 2022 06:25:09 -0500 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id E8ABC248F3; Wed, 9 Nov 2022 03:25:07 -0800 (PST) Received: from neptune (ip5f592f1a.dynamic.kabel-deutschland.de [95.89.47.26]) by linux.microsoft.com (Postfix) with ESMTPSA id 1B3D1205DC1C; Wed, 9 Nov 2022 03:25:02 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 1B3D1205DC1C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1667993107; bh=zUwbg/HEyF+bezTrnaFC6nz76szbZgSmnWrOoGNXm+s=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Hn8MKJT/RjE39bY8km6r3nNO43pooj3ElbJ48yMi2AD+e8wHBh51flFON7siZhHRp mLcXyuPZsf9P+60gg1ewlnJm29k4ElLfV44jMTYN5kss4lYlj/egI9D8+QMTA9NO23 QUtJqAXqxBPJJSvpATB8AyHbswG8r5BqdqtxtMAg= Date: Wed, 9 Nov 2022 12:23:38 +0100 From: Alban Crequy To: Yonghong Song Cc: Francis Laniel , linux-kernel@vger.kernel.org, Alban Crequy , Alban Crequy , Andrew Morton , Andrii Nakryiko , Mykola Lysenko , Alexei Starovoitov , Daniel Borkmann , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , Stanislav Fomichev , Hao Luo , Jiri Olsa , Shuah Khan , linux-mm@kvack.org, bpf@vger.kernel.org, linux-kselftest@vger.kernel.org Subject: Re: [RFC PATCH v1 1/2] maccess: fix writing offset in case of fault in strncpy_from_kernel_nofault() Message-ID: <20221109122338.7e931640@neptune> In-Reply-To: References: <20221108195211.214025-1-flaniel@linux.microsoft.com> <20221108195211.214025-2-flaniel@linux.microsoft.com> Organization: Microsoft X-Mailer: Claws Mail 4.1.1 (GTK 3.24.34; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 8 Nov 2022 12:38:53 -0800 Yonghong Song wrote: > On 11/8/22 12:35 PM, Yonghong Song wrote: > >=20 > >=20 > > On 11/8/22 11:52 AM, Francis Laniel wrote: =20 > >> From: Alban Crequy > >> > >> If a page fault occurs while copying the first byte, this function=20 > >> resets one > >> byte before dst. > >> As a consequence, an address could be modified and leaded to > >> kernel crashes if > >> case the modified address was accessed later. > >> > >> Signed-off-by: Alban Crequy > >> Tested-by: Francis Laniel > >> --- > >> =C2=A0 mm/maccess.c | 2 +- > >> =C2=A0 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/mm/maccess.c b/mm/maccess.c > >> index 5f4d240f67ec..074f6b086671 100644 > >> --- a/mm/maccess.c > >> +++ b/mm/maccess.c > >> @@ -97,7 +97,7 @@ long strncpy_from_kernel_nofault(char *dst, > >> const void *unsafe_addr, long count) > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return src - unsafe_addr; > >> =C2=A0 Efault: > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pagefault_enable(); > >> -=C2=A0=C2=A0=C2=A0 dst[-1] =3D '\0'; > >> +=C2=A0=C2=A0=C2=A0 dst[0] =3D '\0'; =20 > >=20 > > What if the fault is due to dst, so the above won't work, right? > >=20 > > The original code should work fine if the first byte copy is > > successful. For the first byte copy fault, maybe just to leave it > > as is? =20 >=20 > Okay, the dst is always safe (from func signature), so change looks > okay to me. Probably mm people can double check. My understanding was that the bpf verifier is supposed to check that the dst pointer is safe. But I don't know where it is done, and I don't know how it can check that the dst buffer is big enough. > > =20 > >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return -EFAULT; > >> =C2=A0 } > >> > >> --=20 > >> 2.25.1 > >> =20 >=20