[PATCH] x86/sgx: Fix NULL pointer dereference on non-SGX systems

[PATCH] x86/sgx: Fix NULL pointer dereference on non-SGX systems

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Nathan Chancellor reported an oops when aceessing the
'sgx_total_bytes' sysfs file:

	https://lore.kernel.org/all/YbzhBrimHGGpddDM@archlinux-ax161/

The sysfs output code accesses the sgx_numa_nodes[] array
unconditionally.  However, this array is allocated during SGX
initialization, which only occurs on systems where SGX is
supported.

If the sysfs file is accessed on systems without SGX support,
sgx_numa_nodes[] is NULL and an oops occurs.

Add a check to ensure that SGX has been initialized to the point
where sgx_numa_nodes[] is allocated, before accessing it.

Reported-by: Nathan Chancellor <nathan@kernel.org>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: linux-sgx@vger.kernel.org
Cc: x86@kernel.org
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/kernel/cpu/sgx/main.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
--- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr	2021-12-17 13:38:00.217312383 -0800
+++ b/arch/x86/kernel/cpu/sgx/main.c	2021-12-17 14:00:36.293044390 -0800
@@ -906,7 +906,13 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
 #ifdef CONFIG_NUMA
 static ssize_t sgx_total_bytes_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	return sysfs_emit(buf, "%lu\n", sgx_numa_nodes[dev->id].size);
+	unsigned long node_bytes = 0;
+
+	/* Avoid acccessing sgx_numa_nodes[] when it is not allocated: */
+	if (!nodes_empty(sgx_numa_mask))
+		node_bytes = sgx_numa_nodes[dev->id].size;
+
+	return sysfs_emit(buf, "%lu\n", node_bytes);
 }
 static DEVICE_ATTR_RO(sgx_total_bytes);
 
_

Re: [PATCH] x86/sgx: Fix NULL pointer dereference on non-SGX systems

[Nathan Chancellor]

On Fri, Dec 17, 2021 at 02:31:53PM -0800, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> Nathan Chancellor reported an oops when aceessing the
> 'sgx_total_bytes' sysfs file:
> 
> 	https://lore.kernel.org/all/YbzhBrimHGGpddDM@archlinux-ax161/
> 
> The sysfs output code accesses the sgx_numa_nodes[] array
> unconditionally.  However, this array is allocated during SGX
> initialization, which only occurs on systems where SGX is
> supported.
> 
> If the sysfs file is accessed on systems without SGX support,
> sgx_numa_nodes[] is NULL and an oops occurs.
> 
> Add a check to ensure that SGX has been initialized to the point
> where sgx_numa_nodes[] is allocated, before accessing it.
> 
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: linux-sgx@vger.kernel.org
> Cc: x86@kernel.org
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>

Tested-by: Nathan Chancellor <nathan@kernel.org>

> ---
> 
>  b/arch/x86/kernel/cpu/sgx/main.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
> --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr	2021-12-17 13:38:00.217312383 -0800
> +++ b/arch/x86/kernel/cpu/sgx/main.c	2021-12-17 14:00:36.293044390 -0800
> @@ -906,7 +906,13 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  #ifdef CONFIG_NUMA
>  static ssize_t sgx_total_bytes_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> -	return sysfs_emit(buf, "%lu\n", sgx_numa_nodes[dev->id].size);
> +	unsigned long node_bytes = 0;
> +
> +	/* Avoid acccessing sgx_numa_nodes[] when it is not allocated: */
> +	if (!nodes_empty(sgx_numa_mask))
> +		node_bytes = sgx_numa_nodes[dev->id].size;
> +
> +	return sysfs_emit(buf, "%lu\n", node_bytes);
>  }
>  static DEVICE_ATTR_RO(sgx_total_bytes);
>  
> _

Re: [PATCH] x86/sgx: Fix NULL pointer dereference on non-SGX systems

[Greg KH]

On Fri, Dec 17, 2021 at 02:31:53PM -0800, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> Nathan Chancellor reported an oops when aceessing the
> 'sgx_total_bytes' sysfs file:
> 
> 	https://lore.kernel.org/all/YbzhBrimHGGpddDM@archlinux-ax161/
> 
> The sysfs output code accesses the sgx_numa_nodes[] array
> unconditionally.  However, this array is allocated during SGX
> initialization, which only occurs on systems where SGX is
> supported.
> 
> If the sysfs file is accessed on systems without SGX support,
> sgx_numa_nodes[] is NULL and an oops occurs.
> 
> Add a check to ensure that SGX has been initialized to the point
> where sgx_numa_nodes[] is allocated, before accessing it.
> 
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: linux-sgx@vger.kernel.org
> Cc: x86@kernel.org
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
> 
>  b/arch/x86/kernel/cpu/sgx/main.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
> --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr	2021-12-17 13:38:00.217312383 -0800
> +++ b/arch/x86/kernel/cpu/sgx/main.c	2021-12-17 14:00:36.293044390 -0800
> @@ -906,7 +906,13 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  #ifdef CONFIG_NUMA
>  static ssize_t sgx_total_bytes_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> -	return sysfs_emit(buf, "%lu\n", sgx_numa_nodes[dev->id].size);
> +	unsigned long node_bytes = 0;
> +
> +	/* Avoid acccessing sgx_numa_nodes[] when it is not allocated: */
> +	if (!nodes_empty(sgx_numa_mask))
> +		node_bytes = sgx_numa_nodes[dev->id].size;
> +
> +	return sysfs_emit(buf, "%lu\n", node_bytes);
>  }

Why is this file showing up if we do not have sgx_numa_nodes not
allocated?  It shouldn't even be there to access then.

don't return a fake number, just don't present the sysfs file at all.

thanks,

greg k-h

Re: [PATCH] x86/sgx: Fix NULL pointer dereference on non-SGX systems

[Greg KH]

On Sat, Dec 18, 2021 at 12:15:40AM +0100, Greg KH wrote:
> On Fri, Dec 17, 2021 at 02:31:53PM -0800, Dave Hansen wrote:
> > 
> > From: Dave Hansen <dave.hansen@linux.intel.com>
> > 
> > Nathan Chancellor reported an oops when aceessing the
> > 'sgx_total_bytes' sysfs file:
> > 
> > 	https://lore.kernel.org/all/YbzhBrimHGGpddDM@archlinux-ax161/
> > 
> > The sysfs output code accesses the sgx_numa_nodes[] array
> > unconditionally.  However, this array is allocated during SGX
> > initialization, which only occurs on systems where SGX is
> > supported.
> > 
> > If the sysfs file is accessed on systems without SGX support,
> > sgx_numa_nodes[] is NULL and an oops occurs.
> > 
> > Add a check to ensure that SGX has been initialized to the point
> > where sgx_numa_nodes[] is allocated, before accessing it.
> > 
> > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > Cc: linux-sgx@vger.kernel.org
> > Cc: x86@kernel.org
> > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> > ---
> > 
> >  b/arch/x86/kernel/cpu/sgx/main.c |    8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
> > --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr	2021-12-17 13:38:00.217312383 -0800
> > +++ b/arch/x86/kernel/cpu/sgx/main.c	2021-12-17 14:00:36.293044390 -0800
> > @@ -906,7 +906,13 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
> >  #ifdef CONFIG_NUMA
> >  static ssize_t sgx_total_bytes_show(struct device *dev, struct device_attribute *attr, char *buf)
> >  {
> > -	return sysfs_emit(buf, "%lu\n", sgx_numa_nodes[dev->id].size);
> > +	unsigned long node_bytes = 0;
> > +
> > +	/* Avoid acccessing sgx_numa_nodes[] when it is not allocated: */
> > +	if (!nodes_empty(sgx_numa_mask))
> > +		node_bytes = sgx_numa_nodes[dev->id].size;
> > +
> > +	return sysfs_emit(buf, "%lu\n", node_bytes);
> >  }
> 
> Why is this file showing up if we do not have sgx_numa_nodes not
> allocated?  It shouldn't even be there to access then.
> 
> don't return a fake number, just don't present the sysfs file at all.

Or, if you _have_ to have the file present, return an error instead of a
fake value.

But really, only create the file if the system supports it, that's the
rule for sysfs and it makes it very easy to do so (see the is_visible
callback for the attribute group for how to do it.)

thanks,

greg k-h

Re: [PATCH] x86/sgx: Fix NULL pointer dereference on non-SGX systems

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 1521 bytes --]

On 12/17/21 3:30 PM, Greg KH wrote:
>>>
>>> diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
>>> --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr	2021-12-17 13:38:00.217312383 -0800
>>> +++ b/arch/x86/kernel/cpu/sgx/main.c	2021-12-17 14:00:36.293044390 -0800
>>> @@ -906,7 +906,13 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
>>>  #ifdef CONFIG_NUMA
>>>  static ssize_t sgx_total_bytes_show(struct device *dev, struct device_attribute *attr, char *buf)
>>>  {
>>> -	return sysfs_emit(buf, "%lu\n", sgx_numa_nodes[dev->id].size);
>>> +	unsigned long node_bytes = 0;
>>> +
>>> +	/* Avoid acccessing sgx_numa_nodes[] when it is not allocated: */
>>> +	if (!nodes_empty(sgx_numa_mask))
>>> +		node_bytes = sgx_numa_nodes[dev->id].size;
>>> +
>>> +	return sysfs_emit(buf, "%lu\n", node_bytes);
>>>  }
>> Why is this file showing up if we do not have sgx_numa_nodes not
>> allocated?  It shouldn't even be there to access then.
>>
>> don't return a fake number, just don't present the sysfs file at all.
> Or, if you _have_ to have the file present, return an error instead of a
> fake value.
> 
> But really, only create the file if the system supports it, that's the
> rule for sysfs and it makes it very easy to do so (see the is_visible
> callback for the attribute group for how to do it.)

Thanks for the suggestion!  That's a lot nicer.  It's also dirt simple
since we only have one attribute.  Updated patch is attached.

I'll send this out as a real v2 soon if no other issues pop up.

[-- Attachment #2: sgx-null-ptr.patch --]
[-- Type: text/x-patch, Size: 2031 bytes --]


From: Dave Hansen <dave.hansen@linux.intel.com>

Nathan Chancellor reported an oops when aceessing the
'sgx_total_bytes' sysfs file:

	https://lore.kernel.org/all/YbzhBrimHGGpddDM@archlinux-ax161/

The sysfs output code accesses the sgx_numa_nodes[] array
unconditionally.  However, this array is allocated during SGX
initialization, which only occurs on systems where SGX is
supported.

If the sysfs file is accessed on systems without SGX support,
sgx_numa_nodes[] is NULL and an oops occurs.

To fix this, hide the entire nodeX/x86/ attribute group on
systems without SGX support using the ->is_visible attribute
group callback.

Fixes: 50468e431335 ("x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node")
Reported-by: Nathan Chancellor <nathan@kernel.org>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: linux-sgx@vger.kernel.org
Cc: x86@kernel.org
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/kernel/cpu/sgx/main.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
--- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr	2021-12-17 13:38:00.217312383 -0800
+++ b/arch/x86/kernel/cpu/sgx/main.c	2021-12-17 15:48:10.358932505 -0800
@@ -910,6 +910,13 @@ static ssize_t sgx_total_bytes_show(stru
 }
 static DEVICE_ATTR_RO(sgx_total_bytes);
 
+static umode_t arch_node_attr_is_visible(struct kobject * kobj,
+		struct attribute * attr, int idx)
+{
+	/* Make all x86/ attributes invisible when SGX is not initialized: */
+	return !nodes_empty(sgx_numa_mask);
+}
+
 static struct attribute *arch_node_dev_attrs[] = {
 	&dev_attr_sgx_total_bytes.attr,
 	NULL,
@@ -918,6 +925,7 @@ static struct attribute *arch_node_dev_a
 const struct attribute_group arch_node_dev_group = {
 	.name = "x86",
 	.attrs = arch_node_dev_attrs,
+	.is_visible = arch_node_attr_is_visible,
 };
 #endif /* CONFIG_NUMA */
 
_

Re: [PATCH] x86/sgx: Fix NULL pointer dereference on non-SGX systems

[Greg KH]

On Fri, Dec 17, 2021 at 03:55:44PM -0800, Dave Hansen wrote:
> On 12/17/21 3:30 PM, Greg KH wrote:
> >>>
> >>> diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
> >>> --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr	2021-12-17 13:38:00.217312383 -0800
> >>> +++ b/arch/x86/kernel/cpu/sgx/main.c	2021-12-17 14:00:36.293044390 -0800
> >>> @@ -906,7 +906,13 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
> >>>  #ifdef CONFIG_NUMA
> >>>  static ssize_t sgx_total_bytes_show(struct device *dev, struct device_attribute *attr, char *buf)
> >>>  {
> >>> -	return sysfs_emit(buf, "%lu\n", sgx_numa_nodes[dev->id].size);
> >>> +	unsigned long node_bytes = 0;
> >>> +
> >>> +	/* Avoid acccessing sgx_numa_nodes[] when it is not allocated: */
> >>> +	if (!nodes_empty(sgx_numa_mask))
> >>> +		node_bytes = sgx_numa_nodes[dev->id].size;
> >>> +
> >>> +	return sysfs_emit(buf, "%lu\n", node_bytes);
> >>>  }
> >> Why is this file showing up if we do not have sgx_numa_nodes not
> >> allocated?  It shouldn't even be there to access then.
> >>
> >> don't return a fake number, just don't present the sysfs file at all.
> > Or, if you _have_ to have the file present, return an error instead of a
> > fake value.
> > 
> > But really, only create the file if the system supports it, that's the
> > rule for sysfs and it makes it very easy to do so (see the is_visible
> > callback for the attribute group for how to do it.)
> 
> Thanks for the suggestion!  That's a lot nicer.  It's also dirt simple
> since we only have one attribute.  Updated patch is attached.
> 
> I'll send this out as a real v2 soon if no other issues pop up.

> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> Nathan Chancellor reported an oops when aceessing the
> 'sgx_total_bytes' sysfs file:
> 
> 	https://lore.kernel.org/all/YbzhBrimHGGpddDM@archlinux-ax161/
> 
> The sysfs output code accesses the sgx_numa_nodes[] array
> unconditionally.  However, this array is allocated during SGX
> initialization, which only occurs on systems where SGX is
> supported.
> 
> If the sysfs file is accessed on systems without SGX support,
> sgx_numa_nodes[] is NULL and an oops occurs.
> 
> To fix this, hide the entire nodeX/x86/ attribute group on
> systems without SGX support using the ->is_visible attribute
> group callback.
> 
> Fixes: 50468e431335 ("x86/sgx: Add an attribute for the amount of SGX memory in a NUMA node")
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: linux-sgx@vger.kernel.org
> Cc: x86@kernel.org
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
> 
>  b/arch/x86/kernel/cpu/sgx/main.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
> --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr	2021-12-17 13:38:00.217312383 -0800
> +++ b/arch/x86/kernel/cpu/sgx/main.c	2021-12-17 15:48:10.358932505 -0800
> @@ -910,6 +910,13 @@ static ssize_t sgx_total_bytes_show(stru
>  }
>  static DEVICE_ATTR_RO(sgx_total_bytes);
>  
> +static umode_t arch_node_attr_is_visible(struct kobject * kobj,
> +		struct attribute * attr, int idx)
> +{
> +	/* Make all x86/ attributes invisible when SGX is not initialized: */
> +	return !nodes_empty(sgx_numa_mask);

That's a very odd umode_t return value :)

Did you test this?  What was the mode of the sysfs file that is created?

thanks,

greg k-h

Re: [PATCH] x86/sgx: Fix NULL pointer dereference on non-SGX systems

[Dave Hansen]

On 12/17/21 4:07 PM, Greg KH wrote:
>> +static umode_t arch_node_attr_is_visible(struct kobject * kobj,
>> +		struct attribute * attr, int idx)
>> +{
>> +	/* Make all x86/ attributes invisible when SGX is not initialized: */
>> +	return !nodes_empty(sgx_numa_mask);
> That's a very odd umode_t return value :)
> 
> Did you test this?  What was the mode of the sysfs file that is created?

Hah, I tested the "return 0" side of it.  That half works great!

The "return 1" side, obviously not.  I'll fix that up before sending it
out for real.

Re: [PATCH] x86/sgx: Fix NULL pointer dereference on non-SGX systems

[Jarkko Sakkinen]

On Fri, Dec 17, 2021 at 02:31:53PM -0800, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> Nathan Chancellor reported an oops when aceessing the
> 'sgx_total_bytes' sysfs file:
> 
> 	https://lore.kernel.org/all/YbzhBrimHGGpddDM@archlinux-ax161/
> 
> The sysfs output code accesses the sgx_numa_nodes[] array
> unconditionally.  However, this array is allocated during SGX
> initialization, which only occurs on systems where SGX is
> supported.
> 
> If the sysfs file is accessed on systems without SGX support,
> sgx_numa_nodes[] is NULL and an oops occurs.
> 
> Add a check to ensure that SGX has been initialized to the point
> where sgx_numa_nodes[] is allocated, before accessing it.
> 
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: linux-sgx@vger.kernel.org
> Cc: x86@kernel.org
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
> 
>  b/arch/x86/kernel/cpu/sgx/main.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff -puN arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr arch/x86/kernel/cpu/sgx/main.c
> --- a/arch/x86/kernel/cpu/sgx/main.c~sgx-null-ptr	2021-12-17 13:38:00.217312383 -0800
> +++ b/arch/x86/kernel/cpu/sgx/main.c	2021-12-17 14:00:36.293044390 -0800
> @@ -906,7 +906,13 @@ EXPORT_SYMBOL_GPL(sgx_set_attribute);
>  #ifdef CONFIG_NUMA
>  static ssize_t sgx_total_bytes_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> -	return sysfs_emit(buf, "%lu\n", sgx_numa_nodes[dev->id].size);
> +	unsigned long node_bytes = 0;
> +
> +	/* Avoid acccessing sgx_numa_nodes[] when it is not allocated: */
> +	if (!nodes_empty(sgx_numa_mask))
> +		node_bytes = sgx_numa_nodes[dev->id].size;
> +
> +	return sysfs_emit(buf, "%lu\n", node_bytes);
>  }
>  static DEVICE_ATTR_RO(sgx_total_bytes);
>  
> _

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

[Some latency in response time is because off-work up until end of the year]

/Jarkko