Re: sgx_validate_offset_length bug

[Reinette Chatre <reinette.chatre@intel.com>]

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