linux-kernel - Re:Re: [PATCH v2] kexec: copy only happens before uchunk goes to zero

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] [day] [month] [year] [list]

Date: Thu, 22 Feb 2024 17:24:20 +0800 (CST)
From: "yang.zhang" <gaoshanliukou@....com>
To: "Baoquan He" <bhe@...hat.com>
Cc: ebiederm@...ssion.com, kexec@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org, 
	"yang.zhang" <yang.zhang@...intek.com>
Subject: Re:Re: [PATCH v2] kexec: copy only happens before uchunk goes to
 zero


At 2024-02-22 15:45:38, "Baoquan He" <bhe@...hat.com> wrote:
>On 02/20/24 at 10:11am, yang.zhang wrote:
>> From: "yang.zhang" <yang.zhang@...intek.com>
>> 
>> When load segments, all of the copying and the rest
>> only happens before uchunk goes to zero.
>
>The code change looks good, while the log can be improved with more
>details so that people can more easily get what's wrong and fixed.
>
>===
>When loading segments, ubytes is <= mbytes. When ubytes is exhausted,
>there could be remaining mbytes. Then in the while loop, the buf pointer
>advancing with mchunk will causing meaningless reading even though it
>doesn't harm. 
>
>So let's change to make sure that all of the copying and the rest only
>happens before uchunk goes to zero.
>===
>
>Please take above words as reference to udpate patch log and post v3,
>and you can add:
>

>Acked-by: Baoquan He <bhe@...hat.com>
Thanks, i update the commits and post v3.>
>> 
>> Signed-off-by: yang.zhang <yang.zhang@...intek.com>
>> 
>> ---
>> v1 -> v2:
>> - Only copy before uchunk goes to zero
>> 
>> V1: https://lore.kernel.org/lkml/20240130101802.23850-1-gaoshanliukou@163.com/
>> ---
>>  kernel/kexec_core.c | 44 ++++++++++++++++++++++++--------------------
>>  1 file changed, 24 insertions(+), 20 deletions(-)
>> 
>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>> index d08fc7b5db97..2fc3d0e3715a 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();
>> @@ -866,11 +868,18 @@ static int kimage_load_crash_segment(struct kimage *image,
>>  			memset(ptr + uchunk, 0, mchunk - 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);
>> +		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;
>> +		}
>>  		kexec_flush_icache_page(page);
>>  		kunmap_local(ptr);
>>  		arch_kexec_pre_free_pages(page_address(page), 1);
>> @@ -878,12 +887,7 @@ static int kimage_load_crash_segment(struct kimage *image,
>>  			result = -EFAULT;
>>  			goto out;
>>  		}
>> -		ubytes -= uchunk;
>>  		maddr  += mchunk;
>> -		if (image->file_mode)
>> -			kbuf += mchunk;
>> -		else
>> -			buf += mchunk;
>>  		mbytes -= mchunk;
>>  
>>  		cond_resched();
>> -- 
>> 2.25.1
>>