Re: [PATCH 2/3] RISC-V: use memcpy for kexec_file mode

[liaochang (A)]

在 2021/11/2 5:15, Eric W. Biederman 写道:
> Björn Töpel <bjorn.topel@xxxxxxxxx> writes:
> 
>> On Sat, 30 Oct 2021 at 05:51, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>>>
>>> Liao Chang <liaochang1@xxxxxxxxxx> 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
> 
> .
> 

-- 
BR,
Liao, Chang