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
