Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752138AbbHEDaj (ORCPT ); Tue, 4 Aug 2015 23:30:39 -0400 Received: from mail-pd0-f172.google.com ([209.85.192.172]:35421 "EHLO mail-pd0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751846AbbHEDai convert rfc822-to-8bit (ORCPT ); Tue, 4 Aug 2015 23:30:38 -0400 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2098\)) Subject: Re: [RFC] kcore:change kcore_read to make sure the kernel read is safe From: yalin wang In-Reply-To: <20150804153823.51b8ba7b9307dc663d635907@linux-foundation.org> Date: Wed, 5 Aug 2015 11:30:28 +0800 Cc: Ingo Molnar , dave@sr71.net, David Rientjes , fabf@skynet.be, bhe@redhat.com, open list Content-Transfer-Encoding: 8BIT Message-Id: <9E41D070-24A2-48B8-9AF2-47B575228BF1@gmail.com> References: <593904DC-7E60-4B95-B82F-B50A5024085C@gmail.com> <20150804153823.51b8ba7b9307dc663d635907@linux-foundation.org> To: Andrew Morton X-Mailer: Apple Mail (2.2098) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5447 Lines: 150 > On Aug 5, 2015, at 06:38, Andrew Morton wrote: > > On Tue, 4 Aug 2015 11:37:57 +0800 yalin wang wrote: > >> This change kcore_read() to use __copy_from_user_inatomic() to >> copy data from kernel address, because kern_addr_valid() just make sure >> page table is valid during call it, whne it return, the page table may >> change, for example, like set_fixmap() function will change kernel page >> table, then maybe trigger kernel crash if encounter this unluckily. > > The changelog is a bit hard to follow. How does this version look? > > : read_kcore() does a copy_to_user() from kernel memory. This could cause a > : crash if the source (kernel) address is concurrently unmapped via, say, > : set_fixmap(). The kern_addr_valid() check is racy and won't reliably > : prevent this. > : > : Change kcore_read() to use __copy_from_user_inatomic() via a temporary > : buffer to catch such situations. > > What actually happens when copy_to_user() gets a fault on the source > address? It *could* handle it and return -EFAULT. I forget... > > Also... what is special about this particular copy_to_user()? Isn't > every copy_to_user() in the kernel vulnerable to a concurrent > set_fixmap()? Is it that only read_kcore() will read pages which are > subject to set_fixmap() alteration? > Thanks for your great comment . i agree with your git change log, one more question, at first i only focus on arm64 arch, it only check __user* address during copy_from{to}_user, but other architecther like X86 check both source and dest address in copy_from{to}_user, is there some special reason do like this? in my view , just need check __user* address is enough, and if also have ex_table for kernel address access maybe hide some BUG in kernel , i think kernel don’t need it, or am i miss something ? if copy_from{to}_user both check source and dest address, we don’t need this patch, it is safe . Maybe we need one more API , like: copy_data_in_user(__user *source, __user *dest, size_t size) ?? >> --- a/fs/proc/kcore.c >> +++ b/fs/proc/kcore.c >> @@ -86,8 +86,8 @@ static size_t get_kcore_size(int *nphdr, size_t *elf_buflen) >> size = try; >> *nphdr = *nphdr + 1; >> } >> - *elf_buflen = sizeof(struct elfhdr) + >> - (*nphdr + 2)*sizeof(struct elf_phdr) + >> + *elf_buflen = sizeof(struct elfhdr) + >> + (*nphdr + 2)*sizeof(struct elf_phdr) + > > Unrelated whitespace fixes really shouldn't be in here. They don't > bother me too much, but some people get upset ;) > i will seperate in another patch for format correctness. >> 3 * ((sizeof(struct elf_note)) + >> roundup(sizeof(CORE_STR), 4)) + >> roundup(sizeof(struct elf_prstatus), 4) + >> @@ -435,6 +435,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) >> size_t elf_buflen; >> int nphdr; >> unsigned long start; >> + unsigned long page = 0; > > "page" isn't a very good name - when we see that identifier we expect > it to be a `struct page *'. Maybe call it copy_buf or something. > > (And incoming argument "buffer" was poorly named. "buffer" implies some > temporary intermediate thing, which is inappropriate here!) > will change name. >> read_lock(&kclist_lock); >> size = get_kcore_size(&nphdr, &elf_buflen); >> @@ -485,7 +486,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) >> start = kc_offset_to_vaddr(*fpos - elf_buflen); >> if ((tsz = (PAGE_SIZE - (start & ~PAGE_MASK))) > buflen) >> tsz = buflen; >> - >> + >> while (buflen) { >> struct kcore_list *m; >> >> @@ -515,15 +516,32 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) >> } else { >> if (kern_addr_valid(start)) { > > Do we still need the (racy) kern_addr_valid() test? The code should > work OK if this is removed? > Yes, can remove >> unsigned long n; >> + mm_segment_t old_fs = get_fs(); >> + >> + if (page == 0) { >> + page = __get_free_page(GFP_KERNEL); >> + if (page == 0) >> + return -ENOMEM; >> >> - n = copy_to_user(buffer, (char *)start, tsz); >> + } >> + set_fs(KERNEL_DS); >> + pagefault_disable(); >> + n = __copy_from_user_inatomic((void *)page, >> + (__force const void __user *)start, >> + tsz); >> + pagefault_enable(); >> + set_fs(old_fs); > > We should have a code comment in here telling people what's going on. > A concurrent set_fixmap() on the source memory is unexpected! Ok. > >> + if (n) >> + memset((void *)page + tsz - n, 0, n); >> + >> + n = copy_to_user(buffer, (char *)page, tsz); >> /* >> * We cannot distinguish between fault on source >> * and fault on destination. When this happens >> * we clear too and hope it will trigger the >> * EFAULT again. >> */ >> - if (n) { >> + if (n) { >> if (clear_user(buffer + tsz - n, >> n)) >> return -EFAULT; >> @@ -540,7 +558,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) >> start += tsz; >> tsz = (buflen > PAGE_SIZE ? PAGE_SIZE : buflen); >> } >> - >> + free_page(page); >> return acc; >> } > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/