[PATCH 0/2] x86/boot: Fix setup_indirect support

[PATCH 0/2] x86/boot: Fix setup_indirect support

[Ross Philipson]

The setup_indirect support for x86 setup_data was added in November
of 2019. Several issues were found in the implementation and these
two patches were created to address those issues.

These patches were originally posted as part of the larger Trenchboot
patch set but are now being posted separately since they have been
reviewed and can be merged independently of the Trenchboot work.

Ross Philipson (2):
  x86/boot: Fix memremap of setup_indirect structures
  x86/boot: Add setup_indirect support in early_memremap_is_setup_data

 arch/x86/kernel/e820.c     | 31 ++++++++++++++++---------
 arch/x86/kernel/kdebugfs.c | 32 +++++++++++++++++++-------
 arch/x86/kernel/ksysfs.c   | 56 ++++++++++++++++++++++++++++++++++++----------
 arch/x86/kernel/setup.c    | 23 +++++++++++++------
 arch/x86/mm/ioremap.c      | 34 +++++++++++++++++++++++-----
 5 files changed, 132 insertions(+), 44 deletions(-)

-- 
1.8.3.1

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

[Ross Philipson]

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
structures in these cases before accessing them.

Fixes: b3c72fc9a78e ("x86/boot: Introduce setup_indirect")

Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 arch/x86/kernel/e820.c     | 31 ++++++++++++++++---------
 arch/x86/kernel/kdebugfs.c | 32 +++++++++++++++++++-------
 arch/x86/kernel/ksysfs.c   | 56 ++++++++++++++++++++++++++++++++++++----------
 arch/x86/kernel/setup.c    | 23 +++++++++++++------
 arch/x86/mm/ioremap.c      | 13 +++++++----
 5 files changed, 113 insertions(+), 42 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index bc0657f..e023950 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -996,7 +996,8 @@ static int __init parse_memmap_opt(char *str)
 void __init e820__reserve_setup_data(void)
 {
 	struct setup_data *data;
-	u64 pa_data;
+	u64 pa_data, pa_next;
+	u32 len;
 
 	pa_data = boot_params.hdr.setup_data;
 	if (!pa_data)
@@ -1004,6 +1005,9 @@ void __init e820__reserve_setup_data(void)
 
 	while (pa_data) {
 		data = early_memremap(pa_data, sizeof(*data));
+		len = sizeof(*data);
+		pa_next = data->next;
+
 		e820__range_update(pa_data, sizeof(*data)+data->len, E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
 
 		/*
@@ -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);
+
+			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;
 
 	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) {
+				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;
+			} 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;
+				else
+					*size = data->len;
+			} else
 				*size = data->len;
 
 			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;
 		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;
+		} 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);
+		}
+
+		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;
+			}
 		}
 
 		memunmap(data);
-- 
1.8.3.1

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

[Borislav Petkov]

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/

> 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.

> 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?

> +			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.

>  	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.

> +			} 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.

>  		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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Ross Philipson]

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

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

[Borislav Petkov]

On Tue, Feb 15, 2022 at 06:34:43AM -0500, Ross Philipson wrote:
> It can if you run out of slots in the fixed map.

Right. Or if any of the checks in __early_ioremap() fail. But those
would at least warn.

> The only reason I did not check it for NULL was because it was not
> checked elsewhere for NULL.

Elsewhere in the tree or elsewhere in this file or in the setup_indirect
adding code?

> I guess there are two questions:
> 
> 1. Should I also fix it elsewhere in the code I am touching?

Yes pls.

> 2. What should I do on an allocation failure? In a routine like this it
> seems to be a critical early boot failure.

How so?

I'd expect in the case of e820__reserve_setup_data(), for example, to
not call e820__range_update* and not have those indirect ranges present
in the e820 map. What the user intended might not work but it'll at
least boot instead of floating dead in the water.

And similar approach in the other places you're touching.

You could even issue a warning or so so that users at least know what's
going on. I'd say...

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

The original intention?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Ross Philipson]

On 2/15/22 13:37, Borislav Petkov wrote:
> On Tue, Feb 15, 2022 at 06:34:43AM -0500, Ross Philipson wrote:
>> It can if you run out of slots in the fixed map.
> 
> Right. Or if any of the checks in __early_ioremap() fail. But those
> would at least warn.
> 
>> The only reason I did not check it for NULL was because it was not
>> checked elsewhere for NULL.
> 
> Elsewhere in the tree or elsewhere in this file or in the setup_indirect
> adding code?

In the ioremap.c module, the check for NULL is only missing in the
functions we updated but the lack of a check was already there before
these changes went in.

In the setup.c and e820.c modules, the check for NULL is missing in the
functions we updated but the lack of a check was already there before
these changes went in in those functions. The lack of early_memremap()
NULL checks can also be found in other functions in those modules.

> 
>> I guess there are two questions:
>>
>> 1. Should I also fix it elsewhere in the code I am touching?
> 
> Yes pls.
> 
>> 2. What should I do on an allocation failure? In a routine like this it
>> seems to be a critical early boot failure.
> 
> How so?
> 
> I'd expect in the case of e820__reserve_setup_data(), for example, to
> not call e820__range_update* and not have those indirect ranges present
> in the e820 map. What the user intended might not work but it'll at
> least boot instead of floating dead in the water.
> 
> And similar approach in the other places you're touching.

Yes I can see how to handle the failures. I will fix the code to do the
appropriate thing given what each of the functions is doing.

Fixing it in other functions and possibly elsewhere in the arch/x86 code
seems to be out of scope for this patch set. We could send separate
patches and hunt down other places this check is missing.

> 
> You could even issue a warning or so so that users at least know what's
> going on. I'd say...

Yea I can pr_warn when the issue occurs.

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

It was just idle speculation, just ignore this.

Thanks
Ross

> 
> Thx.
> 

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

[Borislav Petkov]

On Tue, Feb 22, 2022 at 04:01:32PM -0500, Ross Philipson wrote:
> In the ioremap.c module, the check for NULL is only missing in the
> functions we updated but the lack of a check was already there before
> these changes went in.
> 
> In the setup.c and e820.c modules, the check for NULL is missing in the
> functions we updated but the lack of a check was already there before
> these changes went in in those functions. The lack of early_memremap()
> NULL checks can also be found in other functions in those modules.

I don't know how to understand this statement: are you saying that,
because there are other cases where - for whatever reason - the retval
check is not taking place - you should not do it either?

Because I can see other places where the return value is checked. I
mean, if the return value check doesn't matter, why not make this
function simply void and not bother at all?

> Fixing it in other functions and possibly elsewhere in the arch/x86 code
> seems to be out of scope for this patch set. We could send separate
> patches and hunt down other places this check is missing.

That would be appreciated.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Ross Philipson]

On 2/24/22 08:42, Borislav Petkov wrote:
> On Tue, Feb 22, 2022 at 04:01:32PM -0500, Ross Philipson wrote:
>> In the ioremap.c module, the check for NULL is only missing in the
>> functions we updated but the lack of a check was already there before
>> these changes went in.
>>
>> In the setup.c and e820.c modules, the check for NULL is missing in the
>> functions we updated but the lack of a check was already there before
>> these changes went in in those functions. The lack of early_memremap()
>> NULL checks can also be found in other functions in those modules.
> 
> I don't know how to understand this statement: are you saying that,
> because there are other cases where - for whatever reason - the retval
> check is not taking place - you should not do it either?
> 
> Because I can see other places where the return value is checked. I
> mean, if the return value check doesn't matter, why not make this
> function simply void and not bother at all?

Sorry if I am being vague. I will try to clarify.

In the functions I made changes to, some already checked return values
from early_memremap and some didn't prior to my changes. In addition,
there are other places/functions in those files that did not check
return values from early_memremap.

I did a search yesterday and found there are other places where the
return value from early_memremap is not checked in files I did not
touch. In general the handling of return values from early_memremap is
very inconsistent throughout arch/x86.

> 
>> Fixing it in other functions and possibly elsewhere in the arch/x86 code
>> seems to be out of scope for this patch set. We could send separate
>> patches and hunt down other places this check is missing.
> 
> That would be appreciated.

I will submit a follow on patch set that chases down all the places
where early_memremap return values are not checked and fix them. In
addition I will do the same for early_ioremap since it seems to have the
same issues throughout arch/x86.

Thanks
Ross

> 
> Thx.
> 

[PATCH 2/2] x86/boot: Add setup_indirect support in early_memremap_is_setup_data

[Ross Philipson]

The x86 boot documentation describes the setup_indirect structures and
how they are used. Only one of the two functions in ioremap.c that needed
to be modified to be aware of the introduction of setup_indirect
functionality was updated. This adds comparable support to the other
function where it was missing.

Fixes: b3c72fc9a78e ("x86/boot: Introduce setup_indirect")

Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 arch/x86/mm/ioremap.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index b45e86e..9129b29 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -670,17 +670,34 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
 
 	paddr = boot_params.hdr.setup_data;
 	while (paddr) {
-		unsigned int len;
+		unsigned int len, size;
 
 		if (phys_addr == paddr)
 			return true;
 
 		data = early_memremap_decrypted(paddr, sizeof(*data));
+		size = sizeof(*data);
 
 		paddr_next = data->next;
 		len = data->len;
 
-		early_memunmap(data, sizeof(*data));
+		if ((phys_addr > paddr) && (phys_addr < (paddr + len))) {
+			early_memunmap(data, sizeof(*data));
+			return true;
+		}
+
+		if (data->type == SETUP_INDIRECT) {
+			size += len;
+			early_memunmap(data, sizeof(*data));
+			data = early_memremap_decrypted(paddr, size);
+
+			if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
+				paddr = ((struct setup_indirect *)data->data)->addr;
+				len = ((struct setup_indirect *)data->data)->len;
+			}
+		}
+
+		early_memunmap(data, size);
 
 		if ((phys_addr > paddr) && (phys_addr < (paddr + len)))
 			return true;
-- 
1.8.3.1

Re: [PATCH 2/2] x86/boot: Add setup_indirect support in early_memremap_is_setup_data

[Borislav Petkov]

On Thu, Jan 27, 2022 at 12:04:16PM -0500, Ross Philipson wrote:
> The x86 boot documentation describes the setup_indirect structures and
> how they are used. Only one of the two functions in ioremap.c that needed
> to be modified to be aware of the introduction of setup_indirect
> functionality was updated. This adds comparable support to the other

s/This adds/Add/

> function where it was missing.
> 
> Fixes: b3c72fc9a78e ("x86/boot: Introduce setup_indirect")
> 

No need for a newline here.

> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  arch/x86/mm/ioremap.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index b45e86e..9129b29 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -670,17 +670,34 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
>  
>  	paddr = boot_params.hdr.setup_data;
>  	while (paddr) {
> -		unsigned int len;
> +		unsigned int len, size;
>  
>  		if (phys_addr == paddr)
>  			return true;
>  
>  		data = early_memremap_decrypted(paddr, sizeof(*data));
> +		size = sizeof(*data);
>  
>  		paddr_next = data->next;
>  		len = data->len;
>  
> -		early_memunmap(data, sizeof(*data));
> +		if ((phys_addr > paddr) && (phys_addr < (paddr + len))) {
> +			early_memunmap(data, sizeof(*data));
> +			return true;
> +		}
> +
> +		if (data->type == SETUP_INDIRECT) {
> +			size += len;
> +			early_memunmap(data, sizeof(*data));
> +			data = early_memremap_decrypted(paddr, size);

That can return NULL.

> +
> +			if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
> +				paddr = ((struct setup_indirect *)data->data)->addr;
> +				len = ((struct setup_indirect *)data->data)->len;

As before, use a helper variable here pls.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/2] x86/boot: Add setup_indirect support in early_memremap_is_setup_data

[Ross Philipson]

On 2/11/22 13:41, Borislav Petkov wrote:
> On Thu, Jan 27, 2022 at 12:04:16PM -0500, Ross Philipson wrote:
>> The x86 boot documentation describes the setup_indirect structures and
>> how they are used. Only one of the two functions in ioremap.c that needed
>> to be modified to be aware of the introduction of setup_indirect
>> functionality was updated. This adds comparable support to the other
> 
> s/This adds/Add/

Ack.

> 
>> function where it was missing.
>>
>> Fixes: b3c72fc9a78e ("x86/boot: Introduce setup_indirect")
>>
> 
> No need for a newline here.
> 
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>> ---
>>  arch/x86/mm/ioremap.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index b45e86e..9129b29 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -670,17 +670,34 @@ static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
>>  
>>  	paddr = boot_params.hdr.setup_data;
>>  	while (paddr) {
>> -		unsigned int len;
>> +		unsigned int len, size;
>>  
>>  		if (phys_addr == paddr)
>>  			return true;
>>  
>>  		data = early_memremap_decrypted(paddr, sizeof(*data));
>> +		size = sizeof(*data);
>>  
>>  		paddr_next = data->next;
>>  		len = data->len;
>>  
>> -		early_memunmap(data, sizeof(*data));
>> +		if ((phys_addr > paddr) && (phys_addr < (paddr + len))) {
>> +			early_memunmap(data, sizeof(*data));
>> +			return true;
>> +		}
>> +
>> +		if (data->type == SETUP_INDIRECT) {
>> +			size += len;
>> +			early_memunmap(data, sizeof(*data));
>> +			data = early_memremap_decrypted(paddr, size);
> 
> That can return NULL.

Although there is a bit more indirection here, it is the same reply with
questions in the first patch.

> 
>> +
>> +			if (((struct setup_indirect *)data->data)->type != SETUP_INDIRECT) {
>> +				paddr = ((struct setup_indirect *)data->data)->addr;
>> +				len = ((struct setup_indirect *)data->data)->len;
> 
> As before, use a helper variable here pls.
> 
> Thx.
> 

Thanks,

Ross Philipson