From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752727AbbHTWQo (ORCPT ); Thu, 20 Aug 2015 18:16:44 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:58007 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751732AbbHTWQn (ORCPT ); Thu, 20 Aug 2015 18:16:43 -0400 Date: Thu, 20 Aug 2015 15:16:41 -0700 From: Andrew Morton To: yalin wang Cc: bp@suse.de, mingo@kernel.org, dave@sr71.net, bhe@redhat.com, linux-kernel@vger.kernel.org, Linus Torvalds Subject: Re: [RFC V2] fs/kcore: change copy_to_user to copy_in_user Message-Id: <20150820151641.2b3f48ee57301a079e4ad538@linux-foundation.org> In-Reply-To: <1440067616-3018-1-git-send-email-yalin.wang2010@gmail.com> References: <1440067616-3018-1-git-send-email-yalin.wang2010@gmail.com> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 20 Aug 2015 18:46:56 +0800 yalin wang wrote: > The copy_to_user() here expect can fix the fault on both kernel and > user address, this is not true on other platforms except x86, > change to user copy_in_user() so that can detect the source and > destination address page fault, work as expected: > > ... > > This is the x86 copy_to_user implementation, the fault instruction is > label 1b, 3b, because movsb instruction will fault on both source and > destination address, but on other platforms, like arm64 and arm: > > ... > > It only check the str instructions, means only check fault on destination > address, if the source address in kernel is invalid, it will result in > kernel panic(), this may happened in read_kcore() function, the > kern_addr_valid() only make sure the address is valid during call it, > when it returned, the kernel address may be invalid, like kunmap(), > setfixmap(), vfree() function will change kernel pagetables, and there is > not any sync between read_kcore() and kernel pagetable change. I change > to use copy_in_user(), so that can check both source and destination address, > avoid the kernel panic() in some unlucky circumstance. This difference between x86 and the other architectures is problematic. It creates risk that code which was tested on x86 will fail on other architectures in rare situations. Now, if I'm understanding things correctly, this situation is rare because there aren't many places in which the kernel will access kernel memory which can be concurrently unmapped. > ... > > --- a/fs/proc/kcore.c > +++ b/fs/proc/kcore.c > @@ -515,8 +515,12 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) > } else { > if (kern_addr_valid(start)) { > unsigned long n; > - > - n = copy_to_user(buffer, (char *)start, tsz); > + if ((start + tsz < tsz) || > + (start + tsz) > TASK_SIZE) > + return -EFAULT; > + set_fs(KERNEL_DS); > + n = copy_in_user(buffer, (char *)start, tsz); > + set_fs(USER_DS); > /* > * We cannot distinguish between fault on source > * and fault on destination. When this happens huh. I seem to have forgotten that copy_in_user() exists. It is of course rigorously undocumented, but iirc it's there to copy from userspace to userspace (copy_within_user would be a better name?) And in this patch what you're doing is repurposing copy_in_user to mean "copy from userspace to kernel memory which might be unmapped". I'm not at all confident that this trick will work on all architectures: code which is designed to handle a userspace fault is now expected to correctly handle a kernel space fault?