linux-kernel - Re: [RFC PATCH v1 1/2] maccess: fix writing offset in case of fault in strncpy_from_kernel

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]

Date:   Wed, 9 Nov 2022 12:23:38 +0100
From:   Alban Crequy <albancrequy@...ux.microsoft.com>
To:     Yonghong Song <yhs@...a.com>
Cc:     Francis Laniel <flaniel@...ux.microsoft.com>,
        linux-kernel@...r.kernel.org,
        Alban Crequy <alban.crequy@...il.com>,
        Alban Crequy <albancrequy@...rosoft.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Mykola Lysenko <mykolal@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Shuah Khan <shuah@...nel.org>, linux-mm@...ck.org,
        bpf@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [RFC PATCH v1 1/2] maccess: fix writing offset in case of fault
 in strncpy_from_kernel_nofault()

On Tue, 8 Nov 2022 12:38:53 -0800
Yonghong Song <yhs@...a.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@...rosoft.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@...rosoft.com>
> >> Tested-by: Francis Laniel <flaniel@...ux.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
> >>  
>