On 7/31/19 5:49 PM, Dmitry Safonov wrote:
> Hi Adrian,
>
> On 7/31/19 5:12 PM, Adrian Reber wrote:
> [..]
>> @@ -2530,14 +2530,12 @@ noinline static int copy_clone_args_from_user(struct
>> kernel_clone_args *kargs,
>> struct clone_args __user *uargs,
>> size_t size)
>> {
>> + struct pid_namespace *pid_ns = task_active_pid_ns(current);
>> struct clone_args args;
>>
>> if (unlikely(size > PAGE_SIZE))
>> return -E2BIG;
>>
>> - if (unlikely(size < sizeof(struct clone_args)))
>> - return -EINVAL;
>> -
>
> It might be better to validate it still somehow, but I don't insist.
>
> [..]
>> @@ -2578,11 +2580,16 @@ noinline static int copy_clone_args_from_user(struct
>> kernel_clone_args *kargs,
>>
>> static bool clone3_args_valid(const struct kernel_clone_args *kargs)
>> {
>> - /*
>> - * All lower bits of the flag word are taken.
>> - * Verify that no other unknown flags are passed along.
>> - */
>> - if (kargs->flags & ~CLONE_LEGACY_FLAGS)
>> + /* Verify that no other unknown flags are passed along. */
>> + if (kargs->flags & ~(CLONE_LEGACY_FLAGS | CLONE_SET_TID))
>> + return false;
>> +
>> + /* Fail if set_tid is set without CLONE_SET_TID */
>> + if (kargs->set_tid && !(kargs->flags & CLONE_SET_TID))
>> + return false;
>> +
>> + /* Also fail if set_tid is invalid */
>> + if ((kargs->set_tid <= 0) && (kargs->flags & CLONE_SET_TID))
>> return false;
>
> Sorry for not mentioning it on v1, but I've noticed it only now:
> you check kargs->set_tid even with the legacy-sized kernel_clone_args,
> which is probably some random value on a task's stack?
Self-correction: On kernel stack in copy_clone_args_from_user().
Which may probably be considered as a security leak..
Sorry again for not spotting it in v1.
Thanks,
Dmitry
