| Message ID | 20211217223153.837591E0@davehans-spike.ostc.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Fix NULL pointer dereference on non-SGX systems | expand |
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); > > _
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
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
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.
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
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.
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
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);