From: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
To: Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
Cc: linux-metag-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH 2/3] metag/usercopy: Zero rest of buffer from copy_from_user
Date: Wed, 5 Apr 2017 07:54:14 +0100 [thread overview]
Message-ID: <20170405065414.GL31606@jhogan-linux.le.imgtec.org> (raw)
In-Reply-To: <20170404163157.GO29622-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2261 bytes --]
On Tue, Apr 04, 2017 at 05:31:57PM +0100, Al Viro wrote:
> On Tue, Apr 04, 2017 at 03:46:51PM +0100, James Hogan wrote:
> > @@ -618,7 +617,7 @@ EXPORT_SYMBOL(__copy_user);
> > " GETB D1Ar1,[%1++]\n" \
> > "2: SETB [%0++],D1Ar1\n", \
> > "3: ADD %2,%2,#1\n" \
> > - " SETB [%0++],D1Ar1\n", \
> > + " ADD %0,%0,#1\n", \
>
> Why bother incrementing dst here?
Mainly for consistency with the current expectations of these macros
(otherwise we'd break the unaligned handling of valid copies), but I
think I agree that if the issue mentioned below if fixed before this
patch we could remove these adds, since it'll always result in an
immediate return based on n and retn (and not dst).
> > +/*
> > + * Copy from user to kernel. The return-value is the number of bytes that were
> > + * inaccessible.
> > + */
> > +unsigned long raw_copy_from_user(void *pdst, const void __user *psrc,
> > + unsigned long n)
> > {
> > register char *dst asm ("A0.2") = pdst;
> > register const char __user *src asm ("A1.2") = psrc;
> > @@ -724,7 +721,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc,
> > __asm_copy_from_user_1(dst, src, retn);
> > n--;
> > if (retn)
> > - goto copy_exception_bytes;
> > + return retn + n;
> > }
> > }
>
> Umm... What happens if psrc points to the last byte of an unmapped page,
> with psrc + 1 pointing to valid data? AFAICS, you'll get GETB fail, have
> retn increased to 1, n decremented by 1, then, assuming you end up in that
> byte-copy loop, have GETB at src + 1 *succeed*, the value fetched fed to SETB
> at dst + 1, n decremented again, non-zero retn finally spotted and n + retn
> returned, reporting that you've copied a single byte. Which you certainly
> have, but it's pdst[1] = psrc[1], not pdst[0] = psrc[0].
>
> IOW, you need to check retn after all __asm_copy_from_user_{1,2} in the
> beginning.
Agreed. This is one of the other fixes I mentioned. I'll rearrange to
put that fix first.
> As for topic branch for #1 and #2 - sure, perfectly fine by me. Just give
> me the branch name and I'll pull. But I think the scenario above needs to
> be dealt with...
Thanks for reviewing
Cheers
James
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
prev parent reply other threads:[~2017-04-05 6:54 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-04 14:46 [PATCH 0/3] metag/usercopy: Support RAW_COPY_USER James Hogan
[not found] ` <cover.67b8034e25703fa0c68295f1ac1d7abe4bd1fac9.1491316836.git-series.james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
2017-04-04 14:46 ` [PATCH 1/3] metag/usercopy: Drop unused macros James Hogan
2017-04-04 14:46 ` [PATCH 3/3] metag/usercopy: Switch to RAW_COPY_USER James Hogan
2017-04-04 14:46 ` [PATCH 2/3] metag/usercopy: Zero rest of buffer from copy_from_user James Hogan
[not found] ` <aad3d0b9fb9309ca17a8cf52f3ad1c03b0b17379.1491316836.git-series.james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
2017-04-04 16:31 ` Al Viro
[not found] ` <20170404163157.GO29622-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
2017-04-05 6:54 ` James Hogan [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170405065414.GL31606@jhogan-linux.le.imgtec.org \
--to=james.hogan-1axoqhu6uovqt0dzr+alfa@public.gmane.org \
--cc=linux-metag-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).