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

[Greg KH <gregkh@linuxfoundation.org>]

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