linux-kernel - Re: [PATCH v4 4/4] sched/core: Add permission checks for setting the latency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <9a4132f2-62cc-4132-1c6d-964ed679afc7@linux.ibm.com>
Date:   Tue, 25 Feb 2020 12:17:29 +0530
From:   Parth Shah <parth@...ux.ibm.com>
To:     Qais Yousef <qais.yousef@....com>, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, chris.hyser@...cle.com,
        patrick.bellasi@...bug.net, valentin.schneider@....com,
        tim.c.chen@...ux.intel.com
Cc:     linux-kernel@...r.kernel.org, peterz@...radead.org,
        mingo@...hat.com, David.Laight@...LAB.COM, pjt@...gle.com,
        pavel@....cz, tj@...nel.org, dhaval.giani@...cle.com,
        qperret@...gle.com
Subject: Re: [PATCH v4 4/4] sched/core: Add permission checks for setting the
 latency_nice value



On 2/24/20 6:59 PM, Qais Yousef wrote:
> On 02/24/20 14:29, Parth Shah wrote:
>> Since the latency_nice uses the similar infrastructure as NICE, use the
>> already existing CAP_SYS_NICE security checks for the latency_nice. This
>> should return -EPERM for the non-root user when trying to set the task
>> latency_nice value to any lower than the current value.
>>
>> Signed-off-by: Parth Shah <parth@...ux.ibm.com>
> 
> I'm not against this, so I'm okay if it goes in as is.
> 
> But IMO the definition of this flag is system dependent and I think it's
> prudent to keep it an admin only configuration.
> 
> It'd be hard to predict how normal application could use and depend on this
> feature in the future, which could tie our hand in terms of extending it.
> 

I am fine with this going in too. But just to lie down the fact on single
page and starting the discussion, here are the pros and cons for including
this permission checks:

Pros:
=====
- Having this permission checks will allow only root users to promote the
task, meaning lowering the latency_nice of the task. This is required in
case when the admin has increased the latency_nice value of a task and
non-root user can not lower it.
- In absence of this check, the non-root user can decrease the latency_nice
value against the admin configured value.

Cons:
=====
- This permission check prevents the non-root user to lower the value. This
is a problem when the user itself has increased the latency_nice value in
the past but fails to lower it again.
- After task fork, non-root user cannot lower the inherited child task's
latency_nice value, which might be a problem in the future for extending
this latency_nice ideas for different optimizations.


> I can't argue hard about this though. But I do feel going further and have
> a sched_feature() for each optimization that uses this flag could be necessary
> too.

I agree to your point.


Thanks,
Parth

> 
> Thanks
> 
> --
> Qais Yousef
> 
>> ---
>>  kernel/sched/core.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index e1dc536d4ca3..f883e1d3cd10 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -4887,6 +4887,10 @@ static int __sched_setscheduler(struct task_struct *p,
>>  			return -EINVAL;
>>  		if (attr->sched_latency_nice < MIN_LATENCY_NICE)
>>  			return -EINVAL;
>> +		/* Use the same security checks as NICE */
>> +		if (attr->sched_latency_nice < p->latency_nice &&
>> +		    !can_nice(p, attr->sched_latency_nice))
>> +			return -EPERM;
>>  	}
>>  
>>  	if (pi)
>> -- 
>> 2.17.2
>>