| Message ID | 20211030031832.165457-3-liaochang1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: kexec: add kexec_file_load() support | expand |
Liao Chang <liaochang1@huawei.com> writes: > The pointer to buffer loading kernel binaries is in kernel space for > kexec_fil mode, When copy_from_user copies data from pointer to a block > of memory, it checkes that the pointer is in the user space range, on > RISCV-V that is: > > static inline bool __access_ok(unsigned long addr, unsigned long size) > { > return size <= TASK_SIZE && addr <= TASK_SIZE - size; > } > > and TASK_SIZE is 0x4000000000 for 64-bits, which now causes > copy_from_user to reject the access of the field 'buf' of struct > kexec_segment that is in range [CONFIG_PAGE_OFFSET - VMALLOC_SIZE, > CONFIG_PAGE_OFFSET), is invalid user space pointer. > > This patch fixes this issue by skipping access_ok(), use mempcy() instead. I am a bit confused. Why is machine_kexec ever calling copy_from_user? That seems wrong in all cases. Even worse then having a copy_from_user is having data that you don't know if you should call copy_from_user on. There is most definitely a bug here. Can someone please sort it out without making the kernel guess what kind of memory it is copying from. Eric > Signed-off-by: Liao Chang <liaochang1@huawei.com> > --- > arch/riscv/kernel/machine_kexec.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/kernel/machine_kexec.c b/arch/riscv/kernel/machine_kexec.c > index e6eca271a4d6..4a5db856919b 100644 > --- a/arch/riscv/kernel/machine_kexec.c > +++ b/arch/riscv/kernel/machine_kexec.c > @@ -65,7 +65,9 @@ machine_kexec_prepare(struct kimage *image) > if (image->segment[i].memsz <= sizeof(fdt)) > continue; > > - if (copy_from_user(&fdt, image->segment[i].buf, sizeof(fdt))) > + if (image->file_mode) > + memcpy(&fdt, image->segment[i].buf, sizeof(fdt)); > + else if (copy_from_user(&fdt, image->segment[i].buf, sizeof(fdt))) > continue; > > if (fdt_check_header(&fdt))
On Sat, 30 Oct 2021 at 05:51, Eric W. Biederman <ebiederm@xmission.com> wrote: > > Liao Chang <liaochang1@huawei.com> writes: > > > The pointer to buffer loading kernel binaries is in kernel space for > > kexec_fil mode, When copy_from_user copies data from pointer to a block > > of memory, it checkes that the pointer is in the user space range, on > > RISCV-V that is: > > > > static inline bool __access_ok(unsigned long addr, unsigned long size) > > { > > return size <= TASK_SIZE && addr <= TASK_SIZE - size; > > } > > > > and TASK_SIZE is 0x4000000000 for 64-bits, which now causes > > copy_from_user to reject the access of the field 'buf' of struct > > kexec_segment that is in range [CONFIG_PAGE_OFFSET - VMALLOC_SIZE, > > CONFIG_PAGE_OFFSET), is invalid user space pointer. > > > > This patch fixes this issue by skipping access_ok(), use mempcy() instead. > > I am a bit confused. > > Why is machine_kexec ever calling copy_from_user? That seems wrong in > all cases. > It's not machine_kexec -- it's machine_kexec_prepare, which pulls out the FDT from the image. It looks like MIPS does it similarly. (Caveat: I might be confused as well! ;-)) Björn
Björn Töpel <bjorn.topel@gmail.com> writes: > On Sat, 30 Oct 2021 at 05:51, Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> Liao Chang <liaochang1@huawei.com> writes: >> >> > The pointer to buffer loading kernel binaries is in kernel space for >> > kexec_fil mode, When copy_from_user copies data from pointer to a block >> > of memory, it checkes that the pointer is in the user space range, on >> > RISCV-V that is: >> > >> > static inline bool __access_ok(unsigned long addr, unsigned long size) >> > { >> > return size <= TASK_SIZE && addr <= TASK_SIZE - size; >> > } >> > >> > and TASK_SIZE is 0x4000000000 for 64-bits, which now causes >> > copy_from_user to reject the access of the field 'buf' of struct >> > kexec_segment that is in range [CONFIG_PAGE_OFFSET - VMALLOC_SIZE, >> > CONFIG_PAGE_OFFSET), is invalid user space pointer. >> > >> > This patch fixes this issue by skipping access_ok(), use mempcy() instead. >> >> I am a bit confused. >> >> Why is machine_kexec ever calling copy_from_user? That seems wrong in >> all cases. >> > > It's not machine_kexec -- it's machine_kexec_prepare, which pulls out > the FDT from the image. It looks like MIPS does it similarly. > > (Caveat: I might be confused as well! ;-)) True it is machine_kexec_prepare. But still. copy_from_user does not belong in there. It is not passed a userspace pointer. This looks more like a case for kmap to read a table from the firmware. Even if it someone made sense it definitely does not make sense to make it a conditional copy_from_user. That way lies madness. The entire change is a smell that there is some abstraction that is going wrong, and that abstraction needs to get fixed. Eric
在 2021/11/2 5:15, Eric W. Biederman 写道: > Björn Töpel <bjorn.topel@gmail.com> writes: > >> On Sat, 30 Oct 2021 at 05:51, Eric W. Biederman <ebiederm@xmission.com> wrote: >>> >>> Liao Chang <liaochang1@huawei.com> writes: >>> >>>> The pointer to buffer loading kernel binaries is in kernel space for >>>> kexec_fil mode, When copy_from_user copies data from pointer to a block >>>> of memory, it checkes that the pointer is in the user space range, on >>>> RISCV-V that is: >>>> >>>> static inline bool __access_ok(unsigned long addr, unsigned long size) >>>> { >>>> return size <= TASK_SIZE && addr <= TASK_SIZE - size; >>>> } >>>> >>>> and TASK_SIZE is 0x4000000000 for 64-bits, which now causes >>>> copy_from_user to reject the access of the field 'buf' of struct >>>> kexec_segment that is in range [CONFIG_PAGE_OFFSET - VMALLOC_SIZE, >>>> CONFIG_PAGE_OFFSET), is invalid user space pointer. >>>> >>>> This patch fixes this issue by skipping access_ok(), use mempcy() instead. >>> >>> I am a bit confused. >>> >>> Why is machine_kexec ever calling copy_from_user? That seems wrong in >>> all cases. >>> >> >> It's not machine_kexec -- it's machine_kexec_prepare, which pulls out >> the FDT from the image. It looks like MIPS does it similarly. >> >> (Caveat: I might be confused as well! ;-)) > > True it is machine_kexec_prepare. But still. copy_from_user does not > belong in there. It is not passed a userspace pointer. > > This looks more like a case for kmap to read a table from the firmware. Thanks for all your comments. As I know, these buffer pointed by kexec_segment object here are allocated in userspace and passed into kernel via sys_kexec_load syscall, that is why it uses copy_from_user to read data from these memory, perhaps Nick Kossifids could explain it further. Do you mean it makes sense to remap the pointer to kernel space using API like virt_to_page and kamp,then read data via memcpy, so that no matter which address space the original pointer belongs to,the abstraction will smell better. > > Even if it someone made sense it definitely does not make sense to > make it a conditional copy_from_user. That way lies madness. > > The entire change is a smell that there is some abstraction that is > going wrong, and that abstraction needs to get fixed. > > Eric > > . >
diff --git a/arch/riscv/kernel/machine_kexec.c b/arch/riscv/kernel/machine_kexec.c index e6eca271a4d6..4a5db856919b 100644 --- a/arch/riscv/kernel/machine_kexec.c +++ b/arch/riscv/kernel/machine_kexec.c @@ -65,7 +65,9 @@ machine_kexec_prepare(struct kimage *image) if (image->segment[i].memsz <= sizeof(fdt)) continue; - if (copy_from_user(&fdt, image->segment[i].buf, sizeof(fdt))) + if (image->file_mode) + memcpy(&fdt, image->segment[i].buf, sizeof(fdt)); + else if (copy_from_user(&fdt, image->segment[i].buf, sizeof(fdt))) continue; if (fdt_check_header(&fdt))
The pointer to buffer loading kernel binaries is in kernel space for kexec_fil mode, When copy_from_user copies data from pointer to a block of memory, it checkes that the pointer is in the user space range, on RISCV-V that is: static inline bool __access_ok(unsigned long addr, unsigned long size) { return size <= TASK_SIZE && addr <= TASK_SIZE - size; } and TASK_SIZE is 0x4000000000 for 64-bits, which now causes copy_from_user to reject the access of the field 'buf' of struct kexec_segment that is in range [CONFIG_PAGE_OFFSET - VMALLOC_SIZE, CONFIG_PAGE_OFFSET), is invalid user space pointer. This patch fixes this issue by skipping access_ok(), use mempcy() instead. Signed-off-by: Liao Chang <liaochang1@huawei.com> --- arch/riscv/kernel/machine_kexec.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)