[RFC,v1,1/2] maccess: fix writing offset in case of fault in strncpy_from_kernel_nofault()
| Message ID | 20221108195211.214025-2-flaniel@linux.microsoft.com |
|---|---|
| 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 l7csp2918893wru;
Tue, 8 Nov 2022 11:59:40 -0800 (PST)
X-Google-Smtp-Source:
AMsMyM4osvl+yeBfScjs/Xvte5/PJJDR07h32XrYPHuLsr/mE4JDdJaKpLxRyxjyMRZnNT+FvC16
X-Received: by 2002:a17:906:6086:b0:731:3970:48d0 with SMTP id
t6-20020a170906608600b00731397048d0mr54539943ejj.16.1667937580380;
Tue, 08 Nov 2022 11:59:40 -0800 (PST)
ARC-Seal: i=1; a=rsa-sha256; t=1667937580; cv=none;
d=google.com; s=arc-20160816;
b=HHub8xms5sMrfmcj1EajSWAhLqIy+Bob4gX0w+RDzmHI1VWKBQT7MC5wPYluesAvIP
78GhQgyJNL/FlEwI8P/dhV3h3FNRhfev5YP0shE9qdIx1wwE2eB9DQCM0r2IGy3I+/wL
4+BZz49x+LyiGvkZsB1Ki4VqPjHTloGW8vPVoJ9+h8rDd1QXDcmhQ66kkDLjqGLCemDq
RSsZaKx5/9klF659yFuBc0Hjwl7U844RH5hRDjltlJ0Wuxag9nHkEBXfzgr/FPXuSx6b
ECkid6Hk+4di9USAhXbRD+ASRNvoX/1wuR/qr9UJxrLdn67ew3feS2TfaoBtoeE1uiLQ
jTBw==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;
s=arc-20160816;
h=list-id:precedence:content-transfer-encoding:mime-version
:references:in-reply-to:message-id:date:subject:cc:to:from
:dkim-signature:dkim-filter;
bh=aV1rbRcoT8elrIFvcswWnTb6/2G/r5XMttMTu6HgEWE=;
b=Fx8NROwRQeKRGafw4Usf8hfSOAX1M3mW/NhD8uKuIH9her+AmT5w1prIyicY6LRGCB
7Vw1NZHWVVyOvagzzgQFQ6OPPReDmd80XsVHM4VgWF609+v7Afl9v4vjc5OuWSTztOR4
jy7OWFzzYySWys2fokZYHAgXwWGnVDXg4BMoEHnboCrLIfzgt4q9B9PNRJa0UTFi7OSX
OmFSkrzjZ5yg0x7b31wxmfBh91Qla+q05aD9NwR18tW4f6u7pvm5SiAgBoQAz74GxUgD
MUoeUcWogwnlM6zIxCfuhjWAh8bM1d2/AdtELwYeTapBmDNmpRaCyOdVG57IGv51WJ2O
zbLQ==
ARC-Authentication-Results: i=1; mx.google.com;
dkim=pass header.i=@linux.microsoft.com header.s=default
header.b=rNA91l5N;
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=pass (p=NONE sp=NONE dis=NONE) header.from=linux.microsoft.com
Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20])
by mx.google.com with ESMTP id
p13-20020a05640210cd00b00463bf8a462csi12569264edu.439.2022.11.08.11.59.17;
Tue, 08 Nov 2022 11:59:40 -0800 (PST)
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;
dkim=pass header.i=@linux.microsoft.com header.s=default
header.b=rNA91l5N;
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=pass (p=NONE sp=NONE dis=NONE) header.from=linux.microsoft.com
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S229798AbiKHTw7 (ORCPT <rfc822;tony84727@gmail.com> + 99 others);
Tue, 8 Nov 2022 14:52:59 -0500
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48678 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S229774AbiKHTwz (ORCPT
<rfc822;linux-kernel@vger.kernel.org>);
Tue, 8 Nov 2022 14:52:55 -0500
Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182])
by lindbergh.monkeyblade.net (Postfix) with ESMTP id 120F67057F;
Tue, 8 Nov 2022 11:52:53 -0800 (PST)
Received: from pwmachine.numericable.fr (85-170-25-210.rev.numericable.fr
[85.170.25.210])
by linux.microsoft.com (Postfix) with ESMTPSA id 100FB20B9F81;
Tue, 8 Nov 2022 11:52:48 -0800 (PST)
DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 100FB20B9F81
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com;
s=default; t=1667937172;
bh=aV1rbRcoT8elrIFvcswWnTb6/2G/r5XMttMTu6HgEWE=;
h=From:To:Cc:Subject:Date:In-Reply-To:References:From;
b=rNA91l5NyZRNSqXKVvrOFtfVRTnClAzSHuIB0O3Egn/SS6gqzKq3v7ZKmQBR/jCpy
mgmLPKxd59MfGA18pKlh2CT9mEP1YYypTZe/p83XNEV2xlMr6zqKrK9/5hjRy1klb0
USlF3Ft41AuAO7/QWRSjFyu3ezjq7SIq6BQwfc2U=
From: Francis Laniel <flaniel@linux.microsoft.com>
To: linux-kernel@vger.kernel.org
Cc: Alban Crequy <alban.crequy@gmail.com>,
Alban Crequy <albancrequy@microsoft.com>,
Francis Laniel <flaniel@linux.microsoft.com>,
Andrew Morton <akpm@linux-foundation.org>,
Andrii Nakryiko <andrii@kernel.org>,
Mykola Lysenko <mykolal@fb.com>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Martin KaFai Lau <martin.lau@linux.dev>,
Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@kernel.org>,
Stanislav Fomichev <sdf@google.com>,
Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
Shuah Khan <shuah@kernel.org>, linux-mm@kvack.org,
bpf@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: [RFC PATCH v1 1/2] maccess: fix writing offset in case of fault in
strncpy_from_kernel_nofault()
Date: Tue, 8 Nov 2022 20:52:06 +0100
Message-Id: <20221108195211.214025-2-flaniel@linux.microsoft.com>
X-Mailer: git-send-email 2.25.1
In-Reply-To: <20221108195211.214025-1-flaniel@linux.microsoft.com>
References: <20221108195211.214025-1-flaniel@linux.microsoft.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-Spam-Status: No, score=-19.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
DKIM_VALID,DKIM_VALID_AU,ENV_AND_HDR_SPF_MATCH,RCVD_IN_DNSWL_MED,
SPF_HELO_PASS,SPF_PASS,USER_IN_DEF_DKIM_WL,USER_IN_DEF_SPF_WL
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?1748959316016960123?=
X-GMAIL-MSGID: =?utf-8?q?1748959316016960123?=
|
| Series |
Fix offset when fault occurs in strncpy_from_kernel_nofault()
|
|
Commit Message
Francis Laniel
Nov. 8, 2022, 7:52 p.m. UTC
From: Alban Crequy <albancrequy@microsoft.com> If a page fault occurs while copying the first byte, this function 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 <albancrequy@microsoft.com> Tested-by: Francis Laniel <flaniel@linux.microsoft.com> --- mm/maccess.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.25.1
Comments
On 11/8/22 11:52 AM, Francis Laniel wrote: > From: Alban Crequy <albancrequy@microsoft.com> > > If a page fault occurs while copying the first byte, this function 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 <albancrequy@microsoft.com> > Tested-by: Francis Laniel <flaniel@linux.microsoft.com> > --- > mm/maccess.c | 2 +- > 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) > return src - unsafe_addr; > Efault: > pagefault_enable(); > - dst[-1] = '\0'; > + dst[0] = '\0'; What if the fault is due to dst, so the above won't work, right? 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? > return -EFAULT; > } > > -- > 2.25.1 >
On 11/8/22 12:35 PM, Yonghong Song wrote: > > > On 11/8/22 11:52 AM, Francis Laniel wrote: >> From: Alban Crequy <albancrequy@microsoft.com> >> >> If a page fault occurs while copying the first byte, this function >> 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 <albancrequy@microsoft.com> >> Tested-by: Francis Laniel <flaniel@linux.microsoft.com> >> --- >> mm/maccess.c | 2 +- >> 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) >> return src - unsafe_addr; >> Efault: >> pagefault_enable(); >> - dst[-1] = '\0'; >> + dst[0] = '\0'; > > What if the fault is due to dst, so the above won't work, right? > > 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? Okay, the dst is always safe (from func signature), so change looks okay to me. Probably mm people can double check. > >> return -EFAULT; >> } >> >> -- >> 2.25.1 >>
On Tue, 8 Nov 2022 20:52:06 +0100 Francis Laniel <flaniel@linux.microsoft.com> wrote: > From: Alban Crequy <albancrequy@microsoft.com> > > If a page fault occurs while copying the first byte, this function 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 <albancrequy@microsoft.com> > Tested-by: Francis Laniel <flaniel@linux.microsoft.com> Reviewed-by: Andrew Morton <akpm@linux-foundation.org> Please merge via the bpf tree. This looks potentially nasty. Fortunately only tracing code uses it, but I'm thinking it should have cc:stable and a Fixes:?
Hi. Le mardi 8 novembre 2022, 22:05:51 CET Andrew Morton a écrit : > On Tue, 8 Nov 2022 20:52:06 +0100 Francis Laniel <flaniel@linux.microsoft.com> wrote: > > From: Alban Crequy <albancrequy@microsoft.com> > > > > If a page fault occurs while copying the first byte, this function 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 <albancrequy@microsoft.com> > > Tested-by: Francis Laniel <flaniel@linux.microsoft.com> > > Reviewed-by: Andrew Morton <akpm@linux-foundation.org> > > Please merge via the bpf tree. > > This looks potentially nasty. Fortunately only tracing code uses it, > but I'm thinking it should have cc:stable and a Fixes:? Thank you for the review! Sorry, I thought to add stable list but forgot to add it when sending the series... I will sent a v2 with your review and without rfc tag to, among others, stable.
On Tue, 8 Nov 2022 12:38:53 -0800 Yonghong Song <yhs@meta.com> wrote: > On 11/8/22 12:35 PM, Yonghong Song wrote: > > > > > > On 11/8/22 11:52 AM, Francis Laniel wrote: > >> From: Alban Crequy <albancrequy@microsoft.com> > >> > >> If a page fault occurs while copying the first byte, this function > >> 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 <albancrequy@microsoft.com> > >> Tested-by: Francis Laniel <flaniel@linux.microsoft.com> > >> --- > >> mm/maccess.c | 2 +- > >> 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) > >> return src - unsafe_addr; > >> Efault: > >> pagefault_enable(); > >> - dst[-1] = '\0'; > >> + dst[0] = '\0'; > > > > What if the fault is due to dst, so the above won't work, right? > > > > 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? > > 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. > > > >> return -EFAULT; > >> } > >> > >> -- > >> 2.25.1 > >> >
On 11/9/22 3:23 AM, Alban Crequy wrote: > On Tue, 8 Nov 2022 12:38:53 -0800 > Yonghong Song <yhs@meta.com> wrote: > >> On 11/8/22 12:35 PM, Yonghong Song wrote: >>> >>> >>> On 11/8/22 11:52 AM, Francis Laniel wrote: >>>> From: Alban Crequy <albancrequy@microsoft.com> >>>> >>>> If a page fault occurs while copying the first byte, this function >>>> 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 <albancrequy@microsoft.com> >>>> Tested-by: Francis Laniel <flaniel@linux.microsoft.com> >>>> --- >>>> mm/maccess.c | 2 +- >>>> 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) >>>> return src - unsafe_addr; >>>> Efault: >>>> pagefault_enable(); >>>> - dst[-1] = '\0'; >>>> + dst[0] = '\0'; >>> >>> What if the fault is due to dst, so the above won't work, right? >>> >>> 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? >> >> 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. Yes, the verifier ensures the buffer actually has the capacity for the buffer size. So we are fine here for 'dst' buffer. > >>> >>>> return -EFAULT; >>>> } >>>> >>>> -- >>>> 2.25.1 >>>> >> >
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) return src - unsafe_addr; Efault: pagefault_enable(); - dst[-1] = '\0'; + dst[0] = '\0'; return -EFAULT; }