Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures

[Ross Philipson <ross.philipson@oracle.com>]

On 2/11/22 12:24, Borislav Petkov wrote:
> On Thu, Jan 27, 2022 at 12:04:15PM -0500, Ross Philipson wrote:
>> As documented, the setup_indirect structure is nested inside
>> the setup_data structures in the setup_data list. The code currently
>> accesses the fields inside the setup_indirect structure but only
>> the sizeof(struct setup_data) is being memremapped. No crash
>> occurred but this is just due to how the area is remapped under the
>> covers.
>>
>> The fix is to properly memremap both the setup_data and setup_indirect
> 
> s/The fix is to properly/Properly/

Ack.

> 
>> structures in these cases before accessing them.
>>
>> Fixes: b3c72fc9a78e ("x86/boot: Introduce setup_indirect")
>>
> 
> No need for that space - Fixes belongs with the rest of the tags.

Got it. Will fix in both.

> 
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
>> @@ -1015,18 +1019,23 @@ void __init e820__reserve_setup_data(void)
>>  						 sizeof(*data) + data->len,
>>  						 E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
>>  
>> -		if (data->type == SETUP_INDIRECT &&
>> -		    ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
>> -			e820__range_update(((struct setup_indirect *)data->data)->addr,
>> -					   ((struct setup_indirect *)data->data)->len,
>> -					   E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
>> -			e820__range_update_kexec(((struct setup_indirect *)data->data)->addr,
>> -						 ((struct setup_indirect *)data->data)->len,
>> -						 E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
>> +		if (data->type == SETUP_INDIRECT) {
>> +			len += data->len;
>> +			early_memunmap(data, sizeof(*data));
>> +			data = early_memremap(pa_data, len);
> 
> Do I see it correctly that early_memremap() can return NULL?

It can if you run out of slots in the fixed map. The only reason I did
not check it for NULL was because it was not checked elsewhere for NULL.
I guess there are two questions:

1. Should I also fix it elsewhere in the code I am touching?
2. What should I do on an allocation failure? In a routine like this it
seems to be a critical early boot failure.

I guess the original intention might have been to let it just blow up
since there is no recovery but that is just conjecture...

> 
>> +			if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
>> +				e820__range_update(((struct setup_indirect *)data->data)->addr,
>> +						   ((struct setup_indirect *)data->data)->len,
>> +						   E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
>> +				e820__range_update_kexec(((struct setup_indirect *)data->data)->addr,
>> +							 ((struct setup_indirect *)data->data)->len,
>> +							 E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
>> +			}
>>  		}
>>  
>> -		pa_data = data->next;
>> -		early_memunmap(data, sizeof(*data));
>> +		pa_data = pa_next;
>> +		early_memunmap(data, len);
>>  	}
>>  
>>  	e820__update_table(e820_table);
>> diff --git a/arch/x86/kernel/kdebugfs.c b/arch/x86/kernel/kdebugfs.c
>> index 64b6da9..e5c72d8 100644
>> --- a/arch/x86/kernel/kdebugfs.c
>> +++ b/arch/x86/kernel/kdebugfs.c
>> @@ -92,7 +92,8 @@ static int __init create_setup_data_nodes(struct dentry *parent)
>>  	struct setup_data *data;
>>  	int error;
>>  	struct dentry *d;
>> -	u64 pa_data;
>> +	u64 pa_data, pa_next;
>> +	u32 len;
>>  	int no = 0;
> 
> The tip-tree preferred ordering of variable declarations at the
> beginning of a function is reverse fir tree order::
> 
> 	struct long_struct_name *descriptive_name;
> 	unsigned long foo, bar;
> 	unsigned int tmp;
> 	int ret;
> 
> The above is faster to parse than the reverse ordering::
> 
> 	int ret;
> 	unsigned int tmp;
> 	unsigned long foo, bar;
> 	struct long_struct_name *descriptive_name;
> 
> And even more so than random ordering::
> 
> 	unsigned long foo, bar;
> 	int ret;
> 	struct long_struct_name *descriptive_name;
> 	unsigned int tmp;
> 
> Please fix all cases in your patch.

Will do.

> 
>>  	d = debugfs_create_dir("setup_data", parent);
>> @@ -112,12 +113,27 @@ static int __init create_setup_data_nodes(struct dentry *parent)
>>  			error = -ENOMEM;
>>  			goto err_dir;
>>  		}
>> -
>> -		if (data->type == SETUP_INDIRECT &&
>> -		    ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
>> -			node->paddr = ((struct setup_indirect *)data->data)->addr;
>> -			node->type  = ((struct setup_indirect *)data->data)->type;
>> -			node->len   = ((struct setup_indirect *)data->data)->len;
>> +		pa_next = data->next;
>> +
>> +		if (data->type == SETUP_INDIRECT) {
>> +			len = sizeof(*data) + data->len;
>> +			memunmap(data);
>> +			data = memremap(pa_data, len, MEMREMAP_WB);
>> +			if (!data) {
> 
> Yap, you need similar error handling above.
> 
>> +				kfree(node);
>> +				error = -ENOMEM;
>> +				goto err_dir;
>> +			}
>> +
>> +			if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
>> +				node->paddr = ((struct setup_indirect *)data->data)->addr;
>> +				node->type  = ((struct setup_indirect *)data->data)->type;
>> +				node->len   = ((struct setup_indirect *)data->data)->len;
> 
> Pls use a helper variable here to not have this ugly casting on each line.

Will fix here and below.

> 
>> +			} else {
>> +				node->paddr = pa_data;
>> +				node->type  = data->type;
>> +				node->len   = data->len;
>> +			}
>>  		} else {
>>  			node->paddr = pa_data;
>>  			node->type  = data->type;
>> @@ -125,7 +141,7 @@ static int __init create_setup_data_nodes(struct dentry *parent)
>>  		}
>>  
>>  		create_setup_data_node(d, no, node);
>> -		pa_data = data->next;
>> +		pa_data = pa_next;
>>  
>>  		memunmap(data);
>>  		no++;
>> diff --git a/arch/x86/kernel/ksysfs.c b/arch/x86/kernel/ksysfs.c
>> index d0a1912..4e8b794 100644
>> --- a/arch/x86/kernel/ksysfs.c
>> +++ b/arch/x86/kernel/ksysfs.c
>> @@ -93,24 +93,35 @@ static int __init get_setup_data_size(int nr, size_t *size)
>>  {
>>  	int i = 0;
>>  	struct setup_data *data;
>> -	u64 pa_data = boot_params.hdr.setup_data;
>> +	u64 pa_data = boot_params.hdr.setup_data, pa_next;
>> +	u32 len;
>>  
>>  	while (pa_data) {
>>  		data = memremap(pa_data, sizeof(*data), MEMREMAP_WB);
>>  		if (!data)
>>  			return -ENOMEM;
>> +		pa_next = data->next;
>> +
>>  		if (nr == i) {
>> -			if (data->type == SETUP_INDIRECT &&
>> -			    ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT)
>> -				*size = ((struct setup_indirect *)data->data)->len;
>> -			else
>> +			if (data->type == SETUP_INDIRECT) {
>> +				len = sizeof(*data) + data->len;
>> +				memunmap(data);
>> +				data = memremap(pa_data, len, MEMREMAP_WB);
>> +				if (!data)
>> +					return -ENOMEM;
>> +
>> +				if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT)
>> +					*size = ((struct setup_indirect *)data->data)->len;
> 
> Ditto.
> 
>> +				else
>> +					*size = data->len;
>> +			} else
>>  				*size = data->len;
> 
> Put the else branch in {} too pls, even if it is a single statement.
> Below too.
> 
>>  
>>  			memunmap(data);
>>  			return 0;
>>  		}
>>  
>> -		pa_data = data->next;
>> +		pa_data = pa_next;
>>  		memunmap(data);
>>  		i++;
>>  	}
>> @@ -122,6 +133,7 @@ static ssize_t type_show(struct kobject *kobj,
>>  {
>>  	int nr, ret;
>>  	u64 paddr;
>> +	u32 len;
>>  	struct setup_data *data;
>>  
>>  	ret = kobj_to_setup_data_nr(kobj, &nr);
>> @@ -135,9 +147,14 @@ static ssize_t type_show(struct kobject *kobj,
>>  	if (!data)
>>  		return -ENOMEM;
>>  
>> -	if (data->type == SETUP_INDIRECT)
>> +	if (data->type == SETUP_INDIRECT) {
>> +		len = sizeof(*data) + data->len;
>> +		memunmap(data);
>> +		data = memremap(paddr, len, MEMREMAP_WB);
>> +		if (!data)
>> +			return -ENOMEM;
> 
> <---- newline here.

Ok.

> 
>>  		ret = sprintf(buf, "0x%x\n", ((struct setup_indirect *)data->data)->type);
>> -	else
>> +	} else
>>  		ret = sprintf(buf, "0x%x\n", data->type);
>>  	memunmap(data);
>>  	return ret;
>> @@ -165,10 +182,25 @@ static ssize_t setup_data_data_read(struct file *fp,
>>  	if (!data)
>>  		return -ENOMEM;
>>  
>> -	if (data->type == SETUP_INDIRECT &&
>> -	    ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
>> -		paddr = ((struct setup_indirect *)data->data)->addr;
>> -		len = ((struct setup_indirect *)data->data)->len;
>> +	if (data->type == SETUP_INDIRECT) {
>> +		len = sizeof(*data) + data->len;
>> +		memunmap(data);
>> +		data = memremap(paddr, len, MEMREMAP_WB);
>> +		if (!data)
>> +			return -ENOMEM;
>> +
>> +		if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
>> +			paddr = ((struct setup_indirect *)data->data)->addr;
>> +			len = ((struct setup_indirect *)data->data)->len;
> 
> Again a helper var pls.
> 
>> +		} else {
>> +			/*
>> +			 * Even though this is technically undefined, return
>> +			 * the data as though it is a normal setup_data struct.
>> +			 * This will at least allow it to be inspected.
>> +			 */
>> +			paddr += sizeof(*data);
>> +			len = data->len;
>> +		}
>>  	} else {
>>  		paddr += sizeof(*data);
>>  		len = data->len;
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index f7a132e..6e29c20 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -370,20 +370,29 @@ static void __init parse_setup_data(void)
>>  static void __init memblock_x86_reserve_range_setup_data(void)
>>  {
>>  	struct setup_data *data;
>> -	u64 pa_data;
>> +	u64 pa_data, pa_next;
>> +	u32 len;
>>  
>>  	pa_data = boot_params.hdr.setup_data;
>>  	while (pa_data) {
>>  		data = early_memremap(pa_data, sizeof(*data));
>> +		len = sizeof(*data);
>> +		pa_next = data->next;
>> +
>>  		memblock_reserve(pa_data, sizeof(*data) + data->len);
>>  
>> -		if (data->type == SETUP_INDIRECT &&
>> -		    ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT)
>> -			memblock_reserve(((struct setup_indirect *)data->data)->addr,
>> -					 ((struct setup_indirect *)data->data)->len);
>> +		if (data->type == SETUP_INDIRECT) {
>> +			len += data->len;
>> +			early_memunmap(data, sizeof(*data));
>> +			data = early_memremap(pa_data, len);
>>  
>> -		pa_data = data->next;
>> -		early_memunmap(data, sizeof(*data));
>> +			if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT)
>> +				memblock_reserve(((struct setup_indirect *)data->data)->addr,
>> +						 ((struct setup_indirect *)data->data)->len);
> 
> Ditto.
> 
>> +		}
>> +
>> +		pa_data = pa_next;
>> +		early_memunmap(data, len);
>>  	}
>>  }
>>  
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 026031b..b45e86e 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -636,10 +636,15 @@ static bool memremap_is_setup_data(resource_size_t phys_addr,
>>  			return true;
>>  		}
>>  
>> -		if (data->type == SETUP_INDIRECT &&
>> -		    ((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
>> -			paddr = ((struct setup_indirect *)data->data)->addr;
>> -			len = ((struct setup_indirect *)data->data)->len;
>> +		if (data->type == SETUP_INDIRECT) {
>> +			memunmap(data);
>> +			data = memremap(paddr, sizeof(*data) + len,
>> +					MEMREMAP_WB | MEMREMAP_DEC);
>> +
>> +			if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
>> +				paddr = ((struct setup_indirect *)data->data)->addr;
>> +				len = ((struct setup_indirect *)data->data)->len;
> 
> Ditto.
> 
> Thx.
> 

Thank you very much for the review.

Ross Philipson