Re: [RFC PATCH v1 1/2] maccess: fix writing offset in case of fault in strncpy_from_kernel_nofault()

[Alban Crequy <albancrequy@linux.microsoft.com>]

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
> >>  
>