Re: [PATCH 6/6] io_uring: allow registration of per-task restrictions

[Jens Axboe <axboe@kernel.dk>]

>> +static int io_register_restrictions_task(void __user *arg, unsigned int nr_args)
>> +{
>> +	struct io_uring_task_restriction __user *ures = arg;
>> +	struct io_uring_task_restriction tres;
>> +	struct io_restriction *res;
>> +	int ret;
> 
> You almost certainly want to copy the seccomp logic of disallowing the
> setting of restrictions unless no_new_privs is set or the process has
> CAP_SYS_ADMIN.

(this is why I missed it, it's 288 lines down of quoted email?)

Good point, yes can do.

> While seccomp is more dangerous in this respect (as it allows you to
> modify the return value of a syscall), being able to alter the execution
> of setuid binaries usually leads to security issues, so it's probably
> best to just copy what seccomp does here.

Agree, that was kind of my goal, just largely mimic that part to avoid
surprises.

>> +	/* Disallow if task already has registered restrictions */
>> +	if (current->io_uring_restrict)
>> +		return -EPERM;
> 
> I guess specifying "stacked" restrictions (a-la seccomp) is intended as
> future work?

You can stack already, you just stack within the current set.

> This is kind of critical for both nesting use-cases and for making this
> usable more widely (I imagine systemd will want to set system-wide
> restrictions which would lock out programs from being able to set their
> own process-wide restrictions -- nested containers are also a fairly
> common use-case these days too).
> 
> (For containers we would probably only really use the cBPF stuff but it
> would be nice for them to both be stackable -- if only for the reason
> that you could set them in any order.)

Agree and this is why you can already do that.

>> +	if (nr_args != 1)
>> +		return -EINVAL;
>> +
>> +	if (copy_from_user(&tres, arg, sizeof(tres)))
>> +		return -EFAULT;
>> +
>> +	if (tres.flags)
>> +		return -EINVAL;
>> +	if (!mem_is_zero(tres.resv, sizeof(tres.resv)))
>> +		return -EINVAL;
> 
> I would suggest using copy_struct_from_user() to make extensions easier,
> but I don't know if that is the kind of thing you feel necessary for
> io_uring APIs.

I don't disagree with that, but the io_uring uapi in this regard has
been extensible in the past with just reserved fields. Hence I'd rather
just stick with that approach, rather than make this case "special".

>> +static int io_register_bpf_filter_task(void __user *arg, unsigned int nr_args)
>> +{
>> +	struct io_restriction *res;
>> +	int ret;
>> +
>> +	if (nr_args != 1)
>> +		return -EINVAL;
> 
> Same comment as above about no_new_privs / CAP_SYS_ADMIN.

Agree, will make that change.

-- 
Jens Axboe