netdev - Re: Latest version of zero-copy fix

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: <52F97D5F.4040100@gentoo.org>
Date:	Mon, 10 Feb 2014 20:31:11 -0500
From:	Richard Yao <ryao@...too.org>
To:	David Miller <davem@...emloft.net>
CC:	netdev@...r.kernel.org
Subject: Re: Latest version of zero-copy fix

On 02/10/2014 06:21 PM, David Miller wrote:
> 
> So now the patch only tests is_vmalloc_addr(), did you test this
> version with the situation that triggers the given backtrace?
> 
> [<ffffffff814878ce>] p9_virtio_zc_request+0x45e/0x510
> [<ffffffff814814ed>] p9_client_zc_rpc.constprop.16+0xfd/0x4f0
> [<ffffffff814839dd>] p9_client_read+0x15d/0x240
> [<ffffffff811c8440>] v9fs_fid_readn+0x50/0xa0
> [<ffffffff811c84a0>] v9fs_file_readn+0x10/0x20
> [<ffffffff811c84e7>] v9fs_file_read+0x37/0x70
> [<ffffffff8114e3fb>] vfs_read+0x9b/0x160
> [<ffffffff81153571>] kernel_read+0x41/0x60
> [<ffffffff810c83ab>] copy_module_from_fd.isra.34+0xfb/0x180
> 
> This is reading from v9fs into a module address.
> 
> It's going generate the same backtrace with your fix.
> 
> I don't think the situation was sufficiently explained to Linus.  In
> fact, I didn't see the above backtrace mentioned at all.
> 

I have tested it against Linux 3.13.0 and I can confirm that it solves
the problem. In fact, this was the original patch in December before I
made the last minute change to is_module_or_vmalloc_addr() only after
deciding that is_vmalloc_addr() was not general enough.

Linus' response had two key points. Linus' first point was that my
generalization of is_vmalloc_addr() to is_module_or_vmalloc_addr() was
unnecessary to fix this problem because the order of events is as follows:

1. We allocate a temporary buffer with vmalloc().
2. We read the module into that buffer.
3. We copy the module from that buffer into its final location.

Linus' second point was that my generalization permitted IO to be done
to a region of memory where IO operations should never be done. When I
wrote this back in December, I wanted to write as general a fix as
possible because Alexander Graf had pointed out to me that there had
Will Deacon had written a patch that should have included this, but
neglected it and I was afraid that that would become a cycle.

Linus' clear explanation of why this is a bad idea changed my mind, so I
submitted the original form of this patch with an updated commit message.

Download attachment "signature.asc" of type "application/pgp-signature" (902 bytes)