| Message ID | c02b60d3b92469a2ccfc0780e974d29da578be73.1664834225.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Minor improvements to sgx_init() | expand |
On Tue, Oct 04, 2022 at 11:04:29AM +1300, Kai Huang wrote: > In sgx_setup_epc_section(), xa_store_range() is called to store EPC > pages' owner section to an Xarray using physical addresses of those EPC > pages as index. Currently, the return value of xa_store_range() is not > checked, but actually it can fail (i.e. due to -ENOMEM). > > Not checking the return value of xa_store_range() would result in the > EPC section being used by SGX driver (and KVM SGX guests), but part or > all of its EPC pages not being handled by the memory failure handling of > EPC page. Such inconsistency should be avoided, even at the cost that > this section won't be used by the kernel. > > Add the missing check of the return value of xa_store_range(), and when > it fails, clean up and fail to initialize the EPC section. > > Fixes: 40e0e7843e23 ("x86/sgx: Add infrastructure to identify SGX EPC pages") > Signed-off-by: Kai Huang <kai.huang@intel.com> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> This needs: Cc: stable@vger.kernel.org # v5.17+ Dave, can you pick this independently of rest of the patch set (unless ofc you have change suggestions)? BR, Jarkko
On Wed, 2022-10-05 at 01:21 +0300, Jarkko Sakkinen wrote: > On Tue, Oct 04, 2022 at 11:04:29AM +1300, Kai Huang wrote: > > In sgx_setup_epc_section(), xa_store_range() is called to store EPC > > pages' owner section to an Xarray using physical addresses of those EPC > > pages as index. Currently, the return value of xa_store_range() is not > > checked, but actually it can fail (i.e. due to -ENOMEM). > > > > Not checking the return value of xa_store_range() would result in the > > EPC section being used by SGX driver (and KVM SGX guests), but part or > > all of its EPC pages not being handled by the memory failure handling of > > EPC page. Such inconsistency should be avoided, even at the cost that > > this section won't be used by the kernel. > > > > Add the missing check of the return value of xa_store_range(), and when > > it fails, clean up and fail to initialize the EPC section. > > > > Fixes: 40e0e7843e23 ("x86/sgx: Add infrastructure to identify SGX EPC pages") > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> > > This needs: > > Cc: stable@vger.kernel.org # v5.17+ > > Dave, can you pick this independently of rest of the patch set > (unless ofc you have change suggestions)? > > BR, Jarkko Thanks Jarkko. I will add the "Cc stable" part if I need to send out a new version.
On Tue, 2022-10-04 at 22:42 +0000, Huang, Kai wrote: > On Wed, 2022-10-05 at 01:21 +0300, Jarkko Sakkinen wrote: > > On Tue, Oct 04, 2022 at 11:04:29AM +1300, Kai Huang wrote: > > > In sgx_setup_epc_section(), xa_store_range() is called to store EPC > > > pages' owner section to an Xarray using physical addresses of those EPC > > > pages as index. Currently, the return value of xa_store_range() is not > > > checked, but actually it can fail (i.e. due to -ENOMEM). > > > > > > Not checking the return value of xa_store_range() would result in the > > > EPC section being used by SGX driver (and KVM SGX guests), but part or > > > all of its EPC pages not being handled by the memory failure handling of > > > EPC page. Such inconsistency should be avoided, even at the cost that > > > this section won't be used by the kernel. > > > > > > Add the missing check of the return value of xa_store_range(), and when > > > it fails, clean up and fail to initialize the EPC section. > > > > > > Fixes: 40e0e7843e23 ("x86/sgx: Add infrastructure to identify SGX EPC pages") > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > This needs: > > > > Cc: stable@vger.kernel.org # v5.17+ > > > > Dave, can you pick this independently of rest of the patch set > > (unless ofc you have change suggestions)? > > > > BR, Jarkko > > Thanks Jarkko. I will add the "Cc stable" part if I need to send out a new > version. > > -- > Thanks, > -Kai > Hi Dave, Is this patch worth to do? For now this should be more like a theoretical issue that I just saw when scanning the code. If it is worth to do I'll send out a new one with Jarkko's reviewed-by and CC stable tag.
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 0fdbc490b0f8..5ddf9d9296f4 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -630,8 +630,12 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, } section->phys_addr = phys_addr; - xa_store_range(&sgx_epc_address_space, section->phys_addr, - phys_addr + size - 1, section, GFP_KERNEL); + if (xa_err(xa_store_range(&sgx_epc_address_space, section->phys_addr, + phys_addr + size - 1, section, GFP_KERNEL))) { + vfree(section->pages); + memunmap(section->virt_addr); + return false; + } for (i = 0; i < nr_pages; i++) { section->pages[i].section = index;
In sgx_setup_epc_section(), xa_store_range() is called to store EPC pages' owner section to an Xarray using physical addresses of those EPC pages as index. Currently, the return value of xa_store_range() is not checked, but actually it can fail (i.e. due to -ENOMEM). Not checking the return value of xa_store_range() would result in the EPC section being used by SGX driver (and KVM SGX guests), but part or all of its EPC pages not being handled by the memory failure handling of EPC page. Such inconsistency should be avoided, even at the cost that this section won't be used by the kernel. Add the missing check of the return value of xa_store_range(), and when it fails, clean up and fail to initialize the EPC section. Fixes: 40e0e7843e23 ("x86/sgx: Add infrastructure to identify SGX EPC pages") Signed-off-by: Kai Huang <kai.huang@intel.com> --- arch/x86/kernel/cpu/sgx/main.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)