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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACT4Y+ZZNqzB5Kq2HM+bvhBNAvaH4oNunpvHM3zfAg0GvZH7zg@mail.gmail.com>
Date:   Tue, 16 May 2017 14:01:51 -0700
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Alexander Potapenko <glider@...gle.com>,
        Kostya Serebryany <kcc@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [iov_iter] use memmove() when copying to/from user page

On Tue, May 16, 2017 at 1:52 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
> On Tue, May 16, 2017 at 01:10:56PM -0700, Dmitry Vyukov wrote:
>
>> >         * behaviour of sendfile() in such a case.  And there I've no problem
>> > with saying "contents after operation is undefined".  If you wish to change
>> > that, by all means start with documenting the semantics you want to promise
>> > to userland.
>>
>> I would say it's already documented.
>> sendfile says that it "copies data". memmove says that it "copies
>> data". memcpy says that it "copies data, but data must not overlap".
>> sendfile does not say that "data must not overlap".
>
> In that case your patch does not suffice.  Overlapping move _forwards_ still
> yields unexpected results, doesn't it?

Why? memmove can move both ways. Do we need to change more memcpy's to
memmove's?


>  I'm all for documenting that
> resulting contents is undefined in case of overlap.  The same goes for write()
> from mmap'ed area, BTW.  You suggest changing that undefined behaviour *and*
> either pretending that it's not undefined anymore (obviously false)

Why is it false?

> or
> failing to describe the new cases when it is not undefined anymore.

I would say it just fixes a bug in current impl. Sendfile docs always
said "sendfile() copies data" (without any but's), but we failed to do
this.


> It's not the cost of extra branch; it's ill-defined rules that would need to
> be followed to be able to rely upon the "improvement".