linux-kernel - Re: [PATCH] kexec: should use uchunk for user buffer increasing

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZcDbpMByJ4VNwMZY@MiWiFi-R3L-srv>
Date: Mon, 5 Feb 2024 20:59:16 +0800
From: Baoquan He <bhe@...hat.com>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: "yang.zhang" <gaoshanliukou@....com>, kexec@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	"yang.zhang" <yang.zhang@...intek.com>
Subject: Re: [PATCH] kexec: should use uchunk for user buffer increasing

On 02/05/24 at 06:27am, Eric W. Biederman wrote:
> Baoquan He <bhe@...hat.com> writes:
> 
> > On 01/30/24 at 06:18pm, yang.zhang wrote:
> >> From: "yang.zhang" <yang.zhang@...intek.com>
> >> 
> >> Because of alignment requirement in kexec-tools, there is
> >> no problem for user buffer increasing when loading segments.
> >> But when coping, the step is uchunk, so we should use uchunk
> >> not mchunk.
> >
> > In theory, ubytes is <= mbytes. So uchunk is always <= mchunk. If ubytes
> > is exhausted, while there's still remaining mbytes, then uchunk is 0,
> > there's still mchunk stepping forward. If I understand it correctly,
> > this is a good catch. Not sure if Eric has comment on this to confirm.
> 
> As far as I can read the code the proposed change is a noop.
> 
> I agree it is more correct to not advance the pointers we read from,
> but since we never read from them after that point it does not
> matter.
> 
> >
> > static int kimage_load_normal_segment(struct kimage *image,
> >                                          struct kexec_segment *segment)
> > {
> > ......
> >
> >                 ptr += maddr & ~PAGE_MASK;
> >                 mchunk = min_t(size_t, mbytes,
> >                                 PAGE_SIZE - (maddr & ~PAGE_MASK));
> >                 uchunk = min(ubytes, mchunk);
> > ......}
> 
> If we are going to improve the code for clarity.  We probably
> want to do something like:
> 
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index d08fc7b5db97..1a8b8ce6bf15 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -800,22 +800,24 @@ static int kimage_load_normal_segment(struct kimage *image,
>                                 PAGE_SIZE - (maddr & ~PAGE_MASK));
>                 uchunk = min(ubytes, mchunk);
>  
> -               /* For file based kexec, source pages are in kernel memory */
> -               if (image->file_mode)
> -                       memcpy(ptr, kbuf, uchunk);
> -               else
> -                       result = copy_from_user(ptr, buf, uchunk);
> +               if (uchunk) {
> +                       /* For file based kexec, source pages are in kernel memory */
> +                       if (image->file_mode)
> +                               memcpy(ptr, kbuf, uchunk);
> +                       else
> +                               result = copy_from_user(ptr, buf, uchunk);
> +                       ubytes -= uchunk;
> +                       if (image->file_mode)
> +                               kbuf += uchunk;
> +                       else
> +                               buf += uchunk;
> +               }
>                 kunmap_local(ptr);
>                 if (result) {
>                         result = -EFAULT;
>                         goto out;
>                 }
> -               ubytes -= uchunk;
>                 maddr  += mchunk;
> -               if (image->file_mode)
> -                       kbuf += mchunk;
> -               else
> -                       buf += mchunk;
>                 mbytes -= mchunk;
>  
>                 cond_resched();
> 
> And make it exceedingly clear that all of the copying and the rest
> only happens before uchunk goes to zero.  Otherwise we are relying
> on a lot of operations becoming noops when uchunk goes to zero.

ACK.
This makes the code logic much clearer, thanks.