Re: [PATCH v7 14/18] x86/boot: make sure KASLR does not step over KHO preserved memory

[Mike Rapoport <rppt@kernel.org>]

On Fri, May 02, 2025 at 11:48:54AM -0700, Dave Hansen wrote:
> On 5/1/25 15:54, Changyuan Lyu wrote:
> > +/*
> > + * If KHO is active, only process its scratch areas to ensure we are not
> > + * stepping onto preserved memory.
> > + */
> > +#ifdef CONFIG_KEXEC_HANDOVER
> > +static bool process_kho_entries(unsigned long minimum, unsigned long image_size)
> > +{
> 
> I thought we agreed to rework this to unconditionally define the
> kho_scratch structures so the #ifdef can go away?

It's either #ifdef or double casting and my understanding was that your
preference was to get rid of the double casting.
 
> > +	struct kho_scratch *kho_scratch;
> > +	struct setup_data *ptr;
> > +	int i, nr_areas = 0;
> > +
> > +	ptr = (struct setup_data *)boot_params_ptr->hdr.setup_data;
> > +	while (ptr) {
> > +		if (ptr->type == SETUP_KEXEC_KHO) {
> > +			struct kho_data *kho = (struct kho_data *)ptr->data;
> > +
> > +			kho_scratch = (void *)kho->scratch_addr;
> > +			nr_areas = kho->scratch_size / sizeof(*kho_scratch);
> > +
> > +			break;
> > +		}
> > +
> > +		ptr = (struct setup_data *)ptr->next;
> > +	}
> > +
> > +	if (!nr_areas)
> > +		return false;
> > +
> > +	for (i = 0; i < nr_areas; i++) {
> > +		struct kho_scratch *area = &kho_scratch[i];
> > +		struct mem_vector region = {
> > +			.start = area->addr,
> > +			.size = area->size,
> > +		};
> > +
> > +		if (process_mem_region(&region, minimum, image_size))
> > +			break;
> > +	}
> > +
> > +	return true;
> > +}
> > +#else
> > +static inline bool process_kho_entries(unsigned long minimum,
> > +				       unsigned long image_size)
> > +{
> > +	return false;
> > +}
> > +#endif
> > +
> >  static unsigned long find_random_phys_addr(unsigned long minimum,
> >  					   unsigned long image_size)
> >  {
> > @@ -775,7 +824,8 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
> >  		return 0;
> >  	}
> >  
> > -	if (!process_efi_entries(minimum, image_size))
> > +	if (!process_kho_entries(minimum, image_size) &&
> > +	    !process_efi_entries(minimum, image_size))
> >  		process_e820_entries(minimum, image_size);
> >  
> >  	phys_addr = slots_fetch_random();
> 
> I made a comment about this in the last round, making this the second
> thing that I've noticed that was not addressed.
> 
> Could you please go back through the last round of comments before you
> repost these?

I presumed that changelog covers it. We'll add a comment here for the next
posting.
 
> Just to be clear: these are making progress, but they're not OK from the
> x86 side yet.

-- 
Sincerely yours,
Mike.