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

[Dave Hansen <dave.hansen@intel.com>]

[-- 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 */
 
_