Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Oct 22, 2020 at 02:42:24PM +0200, David Hildenbrand wrote:
> On 22.10.20 14:18, Greg KH wrote:
> > On Thu, Oct 22, 2020 at 12:48:05PM +0200, Greg KH wrote:
> >> On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
> >>> On 22.10.20 11:32, David Laight wrote:
> >>>> From: David Hildenbrand
> >>>>> Sent: 22 October 2020 10:25
> >>>> ...
> >>>>> ... especially because I recall that clang and gcc behave slightly
> >>>>> differently:
> >>>>>
> >>>>> https://github.com/hjl-tools/x86-psABI/issues/2
> >>>>>
> >>>>> "Function args are different: narrow types are sign or zero extended to
> >>>>> 32 bits, depending on their type. clang depends on this for incoming
> >>>>> args, but gcc doesn't make that assumption. But both compilers do it
> >>>>> when calling, so gcc code can call clang code.
> >>>>
> >>>> It really is best to use 'int' (or even 'long') for all numeric
> >>>> arguments (and results) regardless of the domain of the value.
> >>>>
> >>>> Related, I've always worried about 'bool'....
> >>>>
> >>>>> The upper 32 bits of registers are always undefined garbage for types
> >>>>> smaller than 64 bits."
> >>>>
> >>>> On x86-64 the high bits are zeroed by all 32bit loads.
> >>>
> >>> Yeah, but does not help here.
> >>>
> >>>
> >>> My thinking: if the compiler that calls import_iovec() has garbage in
> >>> the upper 32 bit
> >>>
> >>> a) gcc will zero it out and not rely on it being zero.
> >>> b) clang will not zero it out, assuming it is zero.
> >>>
> >>> But
> >>>
> >>> a) will zero it out when calling the !inlined variant
> >>> b) clang will zero it out when calling the !inlined variant
> >>>
> >>> When inlining, b) strikes. We access garbage. That would mean that we
> >>> have calling code that's not generated by clang/gcc IIUC.
> >>>
> >>> We can test easily by changing the parameters instead of adding an "inline".
> >>
> >> Let me try that as well, as I seem to have a good reproducer, but it
> >> takes a while to run...
> > 
> > Ok, that didn't work.
> > 
> > And I can't seem to "fix" this by adding noinline to patches further
> > along in the patch series (because this commit's function is no longer
> > present due to later patches.)
> 
> We might have the same issues with iovec_from_user() and friends now.
> 
> > 
> > Will keep digging...
> > 
> > greg k-h
> > 
> 
> 
> Might be worth to give this a try, just to see if it's related to
> garbage in upper 32 bit and the way clang is handling it (might be a BUG
> in clang, though):
> 
> 
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 72d88566694e..7527298c6b56 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -267,7 +267,7 @@ size_t hash_and_copy_to_iter(const void *addr,
> size_t bytes, void *hashp,
>                 struct iov_iter *i);
> 
>  struct iovec *iovec_from_user(const struct iovec __user *uvector,
> -               unsigned long nr_segs, unsigned long fast_segs,
> +               unsigned nr_segs, unsigned fast_segs,
>                 struct iovec *fast_iov, bool compat);
>  ssize_t import_iovec(int type, const struct iovec __user *uvec,
>                  unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 1635111c5bd2..58417f1916dc 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1652,7 +1652,7 @@ const void *dup_iter(struct iov_iter *new, struct
> iov_iter *old, gfp_t flags)
>  EXPORT_SYMBOL(dup_iter);
> 
>  static int copy_compat_iovec_from_user(struct iovec *iov,
> -               const struct iovec __user *uvec, unsigned long nr_segs)
> +               const struct iovec __user *uvec, unsigned nr_segs)
>  {
>         const struct compat_iovec __user *uiov =
>                 (const struct compat_iovec __user *)uvec;
> @@ -1684,7 +1684,7 @@ static int copy_compat_iovec_from_user(struct
> iovec *iov,
>  }
> 
>  static int copy_iovec_from_user(struct iovec *iov,
> -               const struct iovec __user *uvec, unsigned long nr_segs)
> +               const struct iovec __user *uvec, unsigned nr_segs)
>  {
>         unsigned long seg;
> 
> @@ -1699,7 +1699,7 @@ static int copy_iovec_from_user(struct iovec *iov,
>  }
> 
>  struct iovec *iovec_from_user(const struct iovec __user *uvec,
> -               unsigned long nr_segs, unsigned long fast_segs,
> +               unsigned nr_segs, unsigned fast_segs,
>                 struct iovec *fast_iov, bool compat)
>  {
>         struct iovec *iov = fast_iov;
> @@ -1738,7 +1738,7 @@ ssize_t __import_iovec(int type, const struct
> iovec __user *uvec,
>                  struct iov_iter *i, bool compat)
>  {
>         ssize_t total_len = 0;
> -       unsigned long seg;
> +       unsigned seg;
>         struct iovec *iov;
> 
>         iov = iovec_from_user(uvec, nr_segs, fast_segs, *iovp, compat);
> 

Ah, I tested the other way around, making everything "unsigned long"
instead.  Will go try this too, as other tests are still running...

thanks,

greg k-h