On 3/10/20 8:01 PM, Eric W. Biederman wrote:
Bernd Edlinger bernd.edlinger@hotmail.de writes:
This changes kcmp_epoll_target to use the new exec_update_mutex instead of cred_guard_mutex.
This should be safe, as the credentials are only used for reading, and furthermore ->mm and ->sighand are updated on execve, but only under the new exec_update_mutex.
Can you add a comment that the exec_update_mutex is not needed for KCMP_FILE? As both sets of credentials during exec are valid for accessing the files so exec_update_mutex does not matter.
some files are closed by do_close_on_exec, so in theory this allows you to examine files that were open in the old user but closed for the new user with either credential.
It is not a race condition, but it may be a security concern.
I don't think exec_update_mutex is needed for KCMP_SYSVSEM or KCMP_EPOLL_TFD either. As I don't think exec changes either one of those.
KCMP_EPOLL_TFD is also accessing file pointers, that is possible.
It might be that KCMP_SYSVSEM is a missed optimization, but I may have overlooked something. I'd rather err on the safe side.
Eric
Signed-off-by: Bernd Edlinger bernd.edlinger@hotmail.de
kernel/kcmp.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/kcmp.c b/kernel/kcmp.c index a0e3d7a..b3ff928 100644 --- a/kernel/kcmp.c +++ b/kernel/kcmp.c @@ -173,8 +173,8 @@ static int kcmp_epoll_target(struct task_struct *task1, /* * One should have enough rights to inspect task details. */
- ret = kcmp_lock(&task1->signal->cred_guard_mutex,
&task2->signal->cred_guard_mutex);
- ret = kcmp_lock(&task1->signal->exec_update_mutex,
&task2->signal->exec_update_mutex);@@ -229,8 +229,8 @@ static int kcmp_epoll_target(struct task_struct *task1, } err_unlock:
- kcmp_unlock(&task1->signal->cred_guard_mutex,
&task2->signal->cred_guard_mutex);
- kcmp_unlock(&task1->signal->exec_update_mutex,
&task2->signal->exec_update_mutex);err: put_task_struct(task1); put_task_struct(task2);