* sgx_validate_offset_length bug @ 2022-10-03 17:19 Borys 2022-10-03 17:33 ` Reinette Chatre 2022-10-04 21:50 ` Jarkko Sakkinen 0 siblings, 2 replies; 8+ messages in thread From: Borys @ 2022-10-03 17:19 UTC (permalink / raw) To: jarkko, dave.hansen, linux-sgx; +Cc: mkow Hi, I've stumbled upon "sgx_validate_offset_length" function in "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7 version), which does not entirely do what it claims. "offset" and "length" parameters are provided by userspace and as such their addition can overflow, which may result in this function approving malicious values. Fortunately this does not result in any exploitable bugs at the moment (or at least I couldn't find any), but this might change if "sgx_validate_offset_length" is used in a new context or current usages are changed, so it might be worth fixing anyway. Simple overflow check `offset + length < offset` should be enough. Best regards, Borys ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sgx_validate_offset_length bug 2022-10-03 17:19 sgx_validate_offset_length bug Borys @ 2022-10-03 17:33 ` Reinette Chatre 2022-10-03 17:58 ` Reinette Chatre 2022-10-04 21:50 ` Jarkko Sakkinen 1 sibling, 1 reply; 8+ messages in thread From: Reinette Chatre @ 2022-10-03 17:33 UTC (permalink / raw) To: Borys, jarkko, dave.hansen, linux-sgx; +Cc: mkow Hi Borys, On 10/3/2022 10:19 AM, Borys wrote: > I've stumbled upon "sgx_validate_offset_length" function in > "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7 > version), which does not entirely do what it claims. "offset" and > "length" parameters are provided by userspace and as such their > addition can overflow, which may result in this function approving > malicious values. Fortunately this does not result in any exploitable > bugs at the moment (or at least I couldn't find any), but this might > change if "sgx_validate_offset_length" is used in a new context or > current usages are changed, so it might be worth fixing anyway. > Simple overflow check `offset + length < offset` should be enough.> Could you please elaborate where you see a possibility for overflow? Together the provided values, offset and length, are already ensured to not exceed the total size of the enclave in the following check: sgx_validate_offset_length() { ... if (offset + length - PAGE_SIZE >= encl->size) return -EINVAL; ... } Reinette ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sgx_validate_offset_length bug 2022-10-03 17:33 ` Reinette Chatre @ 2022-10-03 17:58 ` Reinette Chatre 2022-10-04 13:22 ` Borys 0 siblings, 1 reply; 8+ messages in thread From: Reinette Chatre @ 2022-10-03 17:58 UTC (permalink / raw) To: Borys, jarkko, dave.hansen, linux-sgx; +Cc: mkow Hi Borys, On 10/3/2022 10:33 AM, Reinette Chatre wrote: > On 10/3/2022 10:19 AM, Borys wrote: >> I've stumbled upon "sgx_validate_offset_length" function in >> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7 >> version), which does not entirely do what it claims. "offset" and >> "length" parameters are provided by userspace and as such their >> addition can overflow, which may result in this function approving >> malicious values. Fortunately this does not result in any exploitable >> bugs at the moment (or at least I couldn't find any), but this might >> change if "sgx_validate_offset_length" is used in a new context or >> current usages are changed, so it might be worth fixing anyway. >> Simple overflow check `offset + length < offset` should be enough.> > > Could you please elaborate where you see a possibility for overflow? > > Together the provided values, offset and length, are already ensured to > not exceed the total size of the enclave in the following check: > > sgx_validate_offset_length() { > ... > if (offset + length - PAGE_SIZE >= encl->size) > return -EINVAL; > ... > } I think I see what you mean now ... if offset and length are sufficiently large the above check can still pass but loops that have the following pattern may have issues: for (c = 0 ; c < length; c += PAGE_SIZE) { ... /* do something at <offset> */ } Are you planning to submit a patch for the check you propose? Reinette ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sgx_validate_offset_length bug 2022-10-03 17:58 ` Reinette Chatre @ 2022-10-04 13:22 ` Borys 2022-10-04 15:21 ` Reinette Chatre 0 siblings, 1 reply; 8+ messages in thread From: Borys @ 2022-10-04 13:22 UTC (permalink / raw) To: Reinette Chatre, jarkko, dave.hansen, linux-sgx; +Cc: mkow Hi, On 10/3/22 19:58, Reinette Chatre wrote: > Hi Borys, > > On 10/3/2022 10:33 AM, Reinette Chatre wrote: >> On 10/3/2022 10:19 AM, Borys wrote: >>> I've stumbled upon "sgx_validate_offset_length" function in >>> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7 >>> version), which does not entirely do what it claims. "offset" and >>> "length" parameters are provided by userspace and as such their >>> addition can overflow, which may result in this function approving >>> malicious values. Fortunately this does not result in any exploitable >>> bugs at the moment (or at least I couldn't find any), but this might >>> change if "sgx_validate_offset_length" is used in a new context or >>> current usages are changed, so it might be worth fixing anyway. >>> Simple overflow check `offset + length < offset` should be enough.> >> Could you please elaborate where you see a possibility for overflow? >> >> Together the provided values, offset and length, are already ensured to >> not exceed the total size of the enclave in the following check: >> >> sgx_validate_offset_length() { >> ... >> if (offset + length - PAGE_SIZE >= encl->size) >> return -EINVAL; >> ... >> } > I think I see what you mean now ... if offset and length are > sufficiently large the above check can still pass but loops > that have the following pattern may have issues: > > for (c = 0 ; c < length; c += PAGE_SIZE) { > > ... > /* do something at <offset> */ > > } > > Are you planning to submit a patch for the check you propose? > > Reinette Sure, I'll try to submit a patch later today. Borys ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sgx_validate_offset_length bug 2022-10-04 13:22 ` Borys @ 2022-10-04 15:21 ` Reinette Chatre 0 siblings, 0 replies; 8+ messages in thread From: Reinette Chatre @ 2022-10-04 15:21 UTC (permalink / raw) To: Borys, jarkko, dave.hansen, linux-sgx; +Cc: mkow Hi Borys, On 10/4/2022 6:22 AM, Borys wrote: > On 10/3/22 19:58, Reinette Chatre wrote: >> On 10/3/2022 10:33 AM, Reinette Chatre wrote: >>> On 10/3/2022 10:19 AM, Borys wrote: >>>> I've stumbled upon "sgx_validate_offset_length" function in >>>> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7 >>>> version), which does not entirely do what it claims. "offset" and >>>> "length" parameters are provided by userspace and as such their >>>> addition can overflow, which may result in this function approving >>>> malicious values. Fortunately this does not result in any exploitable >>>> bugs at the moment (or at least I couldn't find any), but this might >>>> change if "sgx_validate_offset_length" is used in a new context or >>>> current usages are changed, so it might be worth fixing anyway. >>>> Simple overflow check `offset + length < offset` should be enough.> >>> Could you please elaborate where you see a possibility for overflow? >>> >>> Together the provided values, offset and length, are already ensured to >>> not exceed the total size of the enclave in the following check: >>> >>> sgx_validate_offset_length() { >>> ... >>> if (offset + length - PAGE_SIZE >= encl->size) >>> return -EINVAL; >>> ... >>> } >> I think I see what you mean now ... if offset and length are >> sufficiently large the above check can still pass but loops >> that have the following pattern may have issues: >> >> for (c = 0 ; c < length; c += PAGE_SIZE) { >> >> ... >> /* do something at <offset> */ >> >> } >> >> Are you planning to submit a patch for the check you propose? >> >> Reinette > > Sure, I'll try to submit a patch later today. Thank you very much. Please do take care when determining the "Fixes" tag. You identified the issue within sgx_validate_offset_length() but please note that this is a function recently introduced by refactoring code that has been in SGX since the beginning. Please see commit: dda03e2c331b ("x86/sgx: Create utility to validate user provided offset and length") While the initial fix will be to sgx_validate_offset_length() care should be taken that the fix also propagates to older kernels that do not have this utility. Either a new fix can be created for older kernels or perhaps the stable team could backport dda03e2c331b together with your fix. There are ways to create a patch that communicates this to stable team's automation. Reinette ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sgx_validate_offset_length bug 2022-10-03 17:19 sgx_validate_offset_length bug Borys 2022-10-03 17:33 ` Reinette Chatre @ 2022-10-04 21:50 ` Jarkko Sakkinen 2022-10-04 23:02 ` Borys 1 sibling, 1 reply; 8+ messages in thread From: Jarkko Sakkinen @ 2022-10-04 21:50 UTC (permalink / raw) To: Borys; +Cc: dave.hansen, linux-sgx, mkow On Mon, Oct 03, 2022 at 07:19:21PM +0200, Borys wrote: > Hi, > > I've stumbled upon "sgx_validate_offset_length" function in > "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7 > version), which does not entirely do what it claims. "offset" and > "length" parameters are provided by userspace and as such their addition > can overflow, which may result in this function approving malicious > values. Fortunately this does not result in any exploitable bugs at the > moment (or at least I couldn't find any), but this might change if > "sgx_validate_offset_length" is used in a new context or current usages > are changed, so it might be worth fixing anyway. Simple overflow check > `offset + length < offset` should be enough. > > Best regards, > > Borys > I agree with the bug but not on security issue. If you can call the ioctl API in the first place, you can already apply the operations in arbitrary locations inside the enclave, i.e. it does not introduce any new capability to the untrusted runtime. BR, Jarkko ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sgx_validate_offset_length bug 2022-10-04 21:50 ` Jarkko Sakkinen @ 2022-10-04 23:02 ` Borys 2022-10-04 23:13 ` Jarkko Sakkinen 0 siblings, 1 reply; 8+ messages in thread From: Borys @ 2022-10-04 23:02 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: dave.hansen, linux-sgx, mkow On 10/4/22 23:50, Jarkko Sakkinen wrote: > On Mon, Oct 03, 2022 at 07:19:21PM +0200, Borys wrote: >> Hi, >> >> I've stumbled upon "sgx_validate_offset_length" function in >> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7 >> version), which does not entirely do what it claims. "offset" and >> "length" parameters are provided by userspace and as such their addition >> can overflow, which may result in this function approving malicious >> values. Fortunately this does not result in any exploitable bugs at the >> moment (or at least I couldn't find any), but this might change if >> "sgx_validate_offset_length" is used in a new context or current usages >> are changed, so it might be worth fixing anyway. Simple overflow check >> `offset + length < offset` should be enough. >> >> Best regards, >> >> Borys >> > > I agree with the bug but not on security issue. > > If you can call the ioctl API in the first place, you can already apply > the operations in arbitrary locations inside the enclave, i.e. it does > not introduce any new capability to the untrusted runtime. > > BR, Jarkko I meant it could possibly enable some local priv escalation, if other code has wrong assumptions. But again, this is purely theoretical, current usages fail on invalid values anyway. Best regards, Borys ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: sgx_validate_offset_length bug 2022-10-04 23:02 ` Borys @ 2022-10-04 23:13 ` Jarkko Sakkinen 0 siblings, 0 replies; 8+ messages in thread From: Jarkko Sakkinen @ 2022-10-04 23:13 UTC (permalink / raw) To: Borys; +Cc: dave.hansen, linux-sgx, mkow On Wed, Oct 05, 2022 at 01:02:39AM +0200, Borys wrote: > On 10/4/22 23:50, Jarkko Sakkinen wrote: > > On Mon, Oct 03, 2022 at 07:19:21PM +0200, Borys wrote: > >> Hi, > >> > >> I've stumbled upon "sgx_validate_offset_length" function in > >> "arch/x86/kernel/cpu/sgx/ioctl.c" (all of this is based on 6.0-rc7 > >> version), which does not entirely do what it claims. "offset" and > >> "length" parameters are provided by userspace and as such their addition > >> can overflow, which may result in this function approving malicious > >> values. Fortunately this does not result in any exploitable bugs at the > >> moment (or at least I couldn't find any), but this might change if > >> "sgx_validate_offset_length" is used in a new context or current usages > >> are changed, so it might be worth fixing anyway. Simple overflow check > >> `offset + length < offset` should be enough. > >> > >> Best regards, > >> > >> Borys > >> > > > > I agree with the bug but not on security issue. > > > > If you can call the ioctl API in the first place, you can already apply > > the operations in arbitrary locations inside the enclave, i.e. it does > > not introduce any new capability to the untrusted runtime. > > > > BR, Jarkko > > I meant it could possibly enable some local priv escalation, if other > code has wrong assumptions. But again, this is purely theoretical, > current usages fail on invalid values anyway. > > Best regards, > Borys Yeah, in all cases it needs to be fixed. Thanks for pointing it out. BR, Jarkko ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-10-04 23:14 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-03 17:19 sgx_validate_offset_length bug Borys 2022-10-03 17:33 ` Reinette Chatre 2022-10-03 17:58 ` Reinette Chatre 2022-10-04 13:22 ` Borys 2022-10-04 15:21 ` Reinette Chatre 2022-10-04 21:50 ` Jarkko Sakkinen 2022-10-04 23:02 ` Borys 2022-10-04 23:13 ` Jarkko Sakkinen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.