| 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
| ||
|
Message-ID: <be0e4730-55d5-6837-65ea-fe6675342309@csgroup.eu>
Date: Sat, 11 Dec 2021 10:32:30 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Tiezhu Yang <yangtiezhu@...ngson.cn>,
Dave Young <dyoung@...hat.com>, Baoquan He <bhe@...hat.com>,
Vivek Goyal <vgoyal@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>
CC: "linux-ia64@...r.kernel.org" <linux-ia64@...r.kernel.org>,
"linux-sh@...r.kernel.org" <linux-sh@...r.kernel.org>,
Xuefeng Li <lixuefeng@...ngson.cn>,
"x86@...nel.org" <x86@...nel.org>,
"kexec@...ts.infradead.org" <kexec@...ts.infradead.org>,
"linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
"linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 1/2] kdump: vmcore: remove copy_to() and add
copy_to_user_or_kernel()
Le 11/12/2021 à 04:33, Tiezhu Yang a écrit :
> In arch/*/kernel/crash_dump*.c, there exist many similar code
> about copy_oldmem_page(), remove copy_to() in fs/proc/vmcore.c
> and add copy_to_user_or_kernel() in lib/usercopy.c, then we can
> use copy_to_user_or_kernel() to simplify the related code.
It should be an inline function in uaccess.h, see below why.
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
> ---
> fs/proc/vmcore.c | 28 +++++++---------------------
> include/linux/uaccess.h | 1 +
> lib/usercopy.c | 15 +++++++++++++++
> 3 files changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> index 509f851..f67fd77 100644
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -238,22 +238,8 @@ copy_oldmem_page_encrypted(unsigned long pfn, char *buf, size_t csize,
> return copy_oldmem_page(pfn, buf, csize, offset, userbuf);
> }
>
> -/*
> - * Copy to either kernel or user space
> - */
> -static int copy_to(void *target, void *src, size_t size, int userbuf)
> -{
> - if (userbuf) {
> - if (copy_to_user((char __user *) target, src, size))
> - return -EFAULT;
> - } else {
> - memcpy(target, src, size);
> - }
> - return 0;
> -}
> -
> #ifdef CONFIG_PROC_VMCORE_DEVICE_DUMP
> -static int vmcoredd_copy_dumps(void *dst, u64 start, size_t size, int userbuf)
> +static int vmcoredd_copy_dumps(void *dst, u64 start, size_t size, bool userbuf)
Changing int to bool in all the callers should be another patch. You can
have copy_to_user_or_kernel() take a bool in the patch while still
having all the callers using an int.
> {
> struct vmcoredd_node *dump;
> u64 offset = 0;
> @@ -266,7 +252,7 @@ static int vmcoredd_copy_dumps(void *dst, u64 start, size_t size, int userbuf)
> if (start < offset + dump->size) {
> tsz = min(offset + (u64)dump->size - start, (u64)size);
> buf = dump->buf + start - offset;
> - if (copy_to(dst, buf, tsz, userbuf)) {
> + if (copy_to_user_or_kernel(dst, buf, tsz, userbuf)) {
> ret = -EFAULT;
> goto out_unlock;
> }
> @@ -330,7 +316,7 @@ static int vmcoredd_mmap_dumps(struct vm_area_struct *vma, unsigned long dst,
> * returned otherwise number of bytes read are returned.
> */
> static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
> - int userbuf)
> + bool userbuf)
> {
> ssize_t acc = 0, tmp;
> size_t tsz;
> @@ -347,7 +333,7 @@ static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
> /* Read ELF core header */
> if (*fpos < elfcorebuf_sz) {
> tsz = min(elfcorebuf_sz - (size_t)*fpos, buflen);
> - if (copy_to(buffer, elfcorebuf + *fpos, tsz, userbuf))
> + if (copy_to_user_or_kernel(buffer, elfcorebuf + *fpos, tsz, userbuf))
> return -EFAULT;
> buflen -= tsz;
> *fpos += tsz;
> @@ -395,7 +381,7 @@ static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
> /* Read remaining elf notes */
> tsz = min(elfcorebuf_sz + elfnotes_sz - (size_t)*fpos, buflen);
> kaddr = elfnotes_buf + *fpos - elfcorebuf_sz - vmcoredd_orig_sz;
> - if (copy_to(buffer, kaddr, tsz, userbuf))
> + if (copy_to_user_or_kernel(buffer, kaddr, tsz, userbuf))
> return -EFAULT;
>
> buflen -= tsz;
> @@ -435,7 +421,7 @@ static ssize_t __read_vmcore(char *buffer, size_t buflen, loff_t *fpos,
> static ssize_t read_vmcore(struct file *file, char __user *buffer,
> size_t buflen, loff_t *fpos)
> {
> - return __read_vmcore((__force char *) buffer, buflen, fpos, 1);
> + return __read_vmcore((__force char *) buffer, buflen, fpos, true);
> }
>
> /*
> @@ -461,7 +447,7 @@ static vm_fault_t mmap_vmcore_fault(struct vm_fault *vmf)
> if (!PageUptodate(page)) {
> offset = (loff_t) index << PAGE_SHIFT;
> buf = __va((page_to_pfn(page) << PAGE_SHIFT));
> - rc = __read_vmcore(buf, PAGE_SIZE, &offset, 0);
> + rc = __read_vmcore(buf, PAGE_SIZE, &offset, false);
> if (rc < 0) {
> unlock_page(page);
> put_page(page);
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index ac03940..a25e682e 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -283,6 +283,7 @@ __copy_from_user_inatomic_nocache(void *to, const void __user *from,
> #endif /* ARCH_HAS_NOCACHE_UACCESS */
>
> extern __must_check int check_zeroed_user(const void __user *from, size_t size);
> +extern __must_check int copy_to_user_or_kernel(void *target, void *src, size_t size, bool userbuf);
extern keyword is pointless for function prototypes, please don't add
new ones.
>
> /**
> * copy_struct_from_user: copy a struct from userspace
> diff --git a/lib/usercopy.c b/lib/usercopy.c
> index 7413dd3..7431b1b 100644
> --- a/lib/usercopy.c
> +++ b/lib/usercopy.c
> @@ -90,3 +90,18 @@ int check_zeroed_user(const void __user *from, size_t size)
> return -EFAULT;
> }
> EXPORT_SYMBOL(check_zeroed_user);
> +
> +/*
> + * Copy to either user or kernel space
> + */
> +int copy_to_user_or_kernel(void *target, void *src, size_t size, bool userbuf)
> +{
> + if (userbuf) {
> + if (copy_to_user((char __user *) target, src, size))
> + return -EFAULT;
> + } else {
> + memcpy(target, src, size);
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(copy_to_user_or_kernel);
>
Ref my answer to Andrew, I don't think outlining this fonction is a
worth it. As shown in that mail, the size of the caller is increased by
4 instructions (which is in the noise) but also this new function is not
small. So I see no benefit in term of size, and I don't think there is
any benefit in terms of performance either.
In this patch that's the same. Before the patch, read_vmcore() has a
size of 0x338.
With this patch, read_vmcore() has a size of 0x340. So that's 2
instructions more, so no benefit either.
So I think this should remain an inline function like in your first
patch (but with the new name).
000001a4 <copy_to_user_or_kernel>:
1a4: 2c 06 00 00 cmpwi r6,0
1a8: 94 21 ff f0 stwu r1,-16(r1)
1ac: 41 82 00 50 beq 1fc <copy_to_user_or_kernel+0x58>
1b0: 2c 05 00 00 cmpwi r5,0
1b4: 41 80 00 7c blt 230 <copy_to_user_or_kernel+0x8c>
1b8: 3d 00 b0 00 lis r8,-20480
1bc: 7f 83 40 40 cmplw cr7,r3,r8
1c0: 41 9c 00 14 blt cr7,1d4 <copy_to_user_or_kernel+0x30>
1c4: 40 82 00 64 bne 228 <copy_to_user_or_kernel+0x84>
1c8: 38 60 00 00 li r3,0
1cc: 38 21 00 10 addi r1,r1,16
1d0: 4e 80 00 20 blr
1d4: 7d 23 40 50 subf r9,r3,r8
1d8: 7f 85 48 40 cmplw cr7,r5,r9
1dc: 7c 08 02 a6 mflr r0
1e0: 90 01 00 14 stw r0,20(r1)
1e4: 41 9d 00 38 bgt cr7,21c <copy_to_user_or_kernel+0x78>
1e8: 48 00 00 01 bl 1e8 <copy_to_user_or_kernel+0x44>
1e8: R_PPC_REL24 __copy_tofrom_user
1ec: 80 01 00 14 lwz r0,20(r1)
1f0: 2c 03 00 00 cmpwi r3,0
1f4: 7c 08 03 a6 mtlr r0
1f8: 4b ff ff cc b 1c4 <copy_to_user_or_kernel+0x20>
1fc: 7c 08 02 a6 mflr r0
200: 90 01 00 14 stw r0,20(r1)
204: 48 00 00 01 bl 204 <copy_to_user_or_kernel+0x60>
204: R_PPC_REL24 memcpy
208: 38 60 00 00 li r3,0
20c: 80 01 00 14 lwz r0,20(r1)
210: 38 21 00 10 addi r1,r1,16
214: 7c 08 03 a6 mtlr r0
218: 4e 80 00 20 blr
21c: 80 01 00 14 lwz r0,20(r1)
220: 7c 08 03 a6 mtlr r0
224: 4b ff ff a0 b 1c4 <copy_to_user_or_kernel+0x20>
228: 38 60 ff f2 li r3,-14
22c: 4b ff ff a0 b 1cc <copy_to_user_or_kernel+0x28>
230: 0f e0 00 00 twui r0,0
234: 7c 08 02 a6 mflr r0
238: 90 01 00 14 stw r0,20(r1)
Also note that checkpatch.pl provides the following on your patch:
CHECK: No space is necessary after a cast
#88: FILE: fs/proc/vmcore.c:424:
+ return __read_vmcore((__force char *) buffer, buflen, fpos, true);
CHECK: extern prototypes should be avoided in .h files
#109: FILE: include/linux/uaccess.h:286:
+extern __must_check int copy_to_user_or_kernel(void *target, void *src,
size_t size, bool userbuf);
CHECK: No space is necessary after a cast
#128: FILE: lib/usercopy.c:100:
+ if (copy_to_user((char __user *) target, src, size))
total: 0 errors, 0 warnings, 3 checks, 96 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or
--fix-inplace.
Commit 2c94767fa768 ("kdump: vmcore: remove copy_to() and add
copy_to_user_or_kernel()") has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
Christophe
Powered by blists - more mailing lists