Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650666 > unrolled thread

Re: [PATCH] [iov_iter] use memmove() when copying to/from user page

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] [iov_iter] use memmove() when copying to/from user page Pavel Machek <pavel@ucw.cz> - 2017-05-25 19:10 +0200
    Re: [PATCH] [iov_iter] use memmove() when copying to/from user page Eric Dumazet <edumazet@google.com> - 2017-05-25 19:20 +0200
      Re: [PATCH] [iov_iter] use memmove() when copying to/from user page Pavel Machek <pavel@ucw.cz> - 2017-05-25 23:30 +0200
    Re: [PATCH] [iov_iter] use memmove() when copying to/from user page Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-25 20:30 +0200
      Re: [PATCH] [iov_iter] use memmove() when copying to/from user page Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-25 21:20 +0200

#1650666 — Re: [PATCH] [iov_iter] use memmove() when copying to/from user page

[Multipart message — attachments visible in raw view] — view raw

Hi!

On Tue 2017-05-16 23:48:54, Al Viro wrote:
> On Tue, May 16, 2017 at 03:15:16PM -0700, Dmitry Vyukov wrote:
> > > Because it's not going to be *one* call of memcpy() or memmove().  It's
> > > one per page.
> > 
> > 
> > I missed that.
> > 
> > I assumed that in the case of sendfile from memfd to memfd data will
> > be copied directly. But it goes through a pipe with multiple buffers.
> > Does not look easily fixable.
> 
> Which leaves us only with "will nasal demons really fly there?".

It seems so:

Date: Tue, 16 May 2017 14:27:34 +0200
From: Alexander Potapenko <glider@google.com>
Subject: [PATCH] [iov_iter] use memmove() when copying to/from user
page

BUG: memcpy-param-overlap in generic_perform_write+0x551/0xa20
__msan_memcpy(ffff88013c6e3001, ffff88013c6e3000, 105)
CPU: 0 PID: 1040 Comm: probe Not tainted 4.11.0-rc5+ #2562
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
01/01/2011

At the very least, we do not want userland to trigger kernel
BUG()s... so this needs fixes beyond documentation.

> Consider the case of write() from an mmapped piece of file to overlapping
> piece of the same file.  It is possible and not hard to trigger; all we
> can guarantee is the lack of infoleaks, filesystem corruption or memory
> corruption.  File *contents* in the affected area can't be sanely relied
> upon.
> 
> This case is not different.  BTW, neither SUS, nor our manpages for
> write(2) mention these issues with mmap()-created aliases between the
> source and destination.

Added people doing documentation; seems like we have some updates to
do there, too.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1650672

On Thu, May 25, 2017 at 10:04 AM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
> On Tue 2017-05-16 23:48:54, Al Viro wrote:
>> On Tue, May 16, 2017 at 03:15:16PM -0700, Dmitry Vyukov wrote:
>> > > Because it's not going to be *one* call of memcpy() or memmove().  It's
>> > > one per page.
>> >
>> >
>> > I missed that.
>> >
>> > I assumed that in the case of sendfile from memfd to memfd data will
>> > be copied directly. But it goes through a pipe with multiple buffers.
>> > Does not look easily fixable.
>>
>> Which leaves us only with "will nasal demons really fly there?".
>
> It seems so:
>
> Date: Tue, 16 May 2017 14:27:34 +0200
> From: Alexander Potapenko <glider@google.com>
> Subject: [PATCH] [iov_iter] use memmove() when copying to/from user
> page
>
> BUG: memcpy-param-overlap in generic_perform_write+0x551/0xa20
> __msan_memcpy(ffff88013c6e3001, ffff88013c6e3000, 105)
> CPU: 0 PID: 1040 Comm: probe Not tainted 4.11.0-rc5+ #2562
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
> 01/01/2011
>
> At the very least, we do not want userland to trigger kernel
> BUG()s... so this needs fixes beyond documentation.

To be fair, this BUG() only happens because Alexander added one in memcpy() ,
testing for the cases where memmove() should have been used.

Kind of a debugging trap if you prefer.

This is not something that a pristine kernel would do.

[toc] | [prev] | [next] | [standalone]

#1650831

[Multipart message — attachments visible in raw view] — view raw

On Thu 2017-05-25 10:15:11, Eric Dumazet wrote:
> On Thu, May 25, 2017 at 10:04 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > Hi!
> >
> > On Tue 2017-05-16 23:48:54, Al Viro wrote:
> >> On Tue, May 16, 2017 at 03:15:16PM -0700, Dmitry Vyukov wrote:
> >> > > Because it's not going to be *one* call of memcpy() or memmove().  It's
> >> > > one per page.
> >> >
> >> >
> >> > I missed that.
> >> >
> >> > I assumed that in the case of sendfile from memfd to memfd data will
> >> > be copied directly. But it goes through a pipe with multiple buffers.
> >> > Does not look easily fixable.
> >>
> >> Which leaves us only with "will nasal demons really fly there?".
> >
> > It seems so:
> >
> > Date: Tue, 16 May 2017 14:27:34 +0200
> > From: Alexander Potapenko <glider@google.com>
> > Subject: [PATCH] [iov_iter] use memmove() when copying to/from user
> > page
> >
> > BUG: memcpy-param-overlap in generic_perform_write+0x551/0xa20
> > __msan_memcpy(ffff88013c6e3001, ffff88013c6e3000, 105)
> > CPU: 0 PID: 1040 Comm: probe Not tainted 4.11.0-rc5+ #2562
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
> > 01/01/2011
> >
> > At the very least, we do not want userland to trigger kernel
> > BUG()s... so this needs fixes beyond documentation.
> 
> To be fair, this BUG() only happens because Alexander added one in memcpy() ,
> testing for the cases where memmove() should have been used.
> 
> Kind of a debugging trap if you prefer.
> 
> This is not something that a pristine kernel would do.

Aha, so BUG() is not realy a problem. Problem is that memcpy() may not
be called on overlapping regions, and may do something stupid; but we
don't have evidence that it does.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1650718

On Thu, May 25, 2017 at 07:04:57PM +0200, Pavel Machek wrote:

> Date: Tue, 16 May 2017 14:27:34 +0200
> From: Alexander Potapenko <glider@google.com>
> Subject: [PATCH] [iov_iter] use memmove() when copying to/from user
> page
> 
> BUG: memcpy-param-overlap in generic_perform_write+0x551/0xa20
> __msan_memcpy(ffff88013c6e3001, ffff88013c6e3000, 105)
> CPU: 0 PID: 1040 Comm: probe Not tainted 4.11.0-rc5+ #2562
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
> 01/01/2011
> 
> At the very least, we do not want userland to trigger kernel
> BUG()s... so this needs fixes beyond documentation.

Sure.  Which is to say, the instrumentation that throws such
BUG() is broken and needs to be fixed.

This is bullshit; the _only_ value memmove() would have here is
"doesn't yield false positives from fuck knows what out-of-tree
patches".

It does not make overlapping sendfile() work reliably (while
creating an impression that it just might, as we'd seen in
this thread).  It does not do anything to kernel-vs-userland
aliasing either - copy_from_user() is definitely memcpy()-like,
not memmove()-like.  Not that memmove() worked in situations
when source and destination point to the same memory object
seen at two virtial addresses...

[toc] | [prev] | [next] | [standalone]

#1650762

On Thu, May 25, 2017 at 07:22:01PM +0100, Al Viro wrote:

> It does not make overlapping sendfile() work reliably (while
> creating an impression that it just might, as we'd seen in
> this thread).  It does not do anything to kernel-vs-userland
> aliasing either - copy_from_user() is definitely memcpy()-like,
> not memmove()-like.  Not that memmove() worked in situations
> when source and destination point to the same memory object
> seen at two virtial addresses...

BTW, the last part goes both for kernel and for userland:

#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
#include <string.h>
#include <sys/mman.h>
#include <sys/stat.h>

void f(char *p, char *q)
{
	memset(p, 0, 4096);
	p[254] = 1;
        memmove(q, p + 127, 256);
        memmove(p, q + 127, 256);
	printf("%d\n", p[0]);
}

main()
{
        static char p[4096];
        int fd = creat("/tmp/foo", 0600);
        unsigned char *p1, *p2;
        int i;

        write(fd, p, 4096);
        close(fd);
        fd = open("/tmp/foo", O_RDWR);
        p1 = mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
        p2 = mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);

	f(p1, p1);
	f(p1, p2);
	f(p2, p1);
}

should've printed 1 three times, according to C99/SuS.  On
stretch/amd64 (with 2.24-10 glibc) we get
1
1

0
instead.  mmap() is out of scope for C99, of course, but not for SuS.
And SuS treatment of memmove() is:

    [CX] [Option Start] The functionality described on this reference
    page is aligned with the ISO C standard. Any conflict between
    the requirements described here and the ISO C standard is
    unintentional. This volume of POSIX.1-2008 defers to the ISO C
    standard. [Option End]

    The memmove() function shall copy n bytes from the object pointed
    to by s2 into the object pointed to by s1. Copying takes place as if
    the n bytes from the object pointed to by s2 are first copied into a
    temporary array of n bytes that does not overlap the objects pointed
    to by s1 and s2, and then the n bytes from the temporary array are
    copied into the object pointed to by s1.

In case when physical memory areas overlap but addresses do not, results
of memmove(3) are impossible to rely upon on all implementations I've
seen.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web