Re: sgx_validate_offset_length bug

[Borys <borysp@invisiblethingslab.com>]

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