[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20201022125759.GA1685526@kroah.com>
Date: Thu, 22 Oct 2020 14:57:59 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: David Hildenbrand <david@...hat.com>
Cc: David Laight <David.Laight@...lab.com>,
Al Viro <viro@...iv.linux.org.uk>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Christoph Hellwig <hch@....de>,
"kernel-team@...roid.com" <kernel-team@...roid.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Jens Axboe <axboe@...nel.dk>, Arnd Bergmann <arnd@...db.de>,
David Howells <dhowells@...hat.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
"linux-parisc@...r.kernel.org" <linux-parisc@...r.kernel.org>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
"linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>,
"sparclinux@...r.kernel.org" <sparclinux@...r.kernel.org>,
"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
"linux-aio@...ck.org" <linux-aio@...ck.org>,
"io-uring@...r.kernel.org" <io-uring@...r.kernel.org>,
"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
"linux-mm@...ck.org" <linux-mm@...ck.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"keyrings@...r.kernel.org" <keyrings@...r.kernel.org>,
"linux-security-module@...r.kernel.org"
<linux-security-module@...r.kernel.org>
Subject: Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move
rw_copy_check_uvector() into lib/iov_iter.c"
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
Powered by blists - more mailing lists