Re: [PATCH] ipc/mqueue.c: change __do_notify() to bypass check_kill_permission()

[Eric W. Biederman]

Oleg Nesterov <oleg@xxxxxxxxxx> writes:

> On 03/22, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@xxxxxxxxxx> writes:
>>
>> > Commit cc731525f26a ("signal: Remove kernel interal si_code magic")
>> > changed the value of SI_FROMUSER(SI_MESGQ), this means that mq_notify()
>> > no longer works if the sender doesn't have rights to send a signal.
>> >
>> > Change __do_notify() to use do_send_sig_info() instead of kill_pid_info()
>> > to avoid check_kill_permission().
>>
>> I totally see why you are doing this.  To avoid the permission check,
>> and since this process requested the signal it makes sense to bypass the
>> permission checks.
>
> And this is what we had before cc731525f26a, so this patch tries to fix
> the regression.

I was intending to suggest a new function that took a pid and did not do
the permission checks.

Looking a little further I think there is a reasonbly strong argument
that this code should be using send_sigqueue with a preallocated signal,
just like timers do.

>> The code needs to make certain that this signal is
>> canceled or otherwise won't be sent after an exec.
>
> not sure I understand this part, but see below.
>
>> That said I don't like it.  I would really like to remove the signal
>> sending interfaces that take a task_struct.
>
> Oh, can we discuss the possible cleanups separately? On top of this fix,
> if possible.

I was saying that from my perspective your proposed fix appears to make
the code more of a mess, and harder to maintain.

That is relevant, because if we can't maintain the code it will just
break again or perhaps get worse.

>> Looking at the code I currently see several places where we have this
>> kind of semantic (sending a requested signal to a process from the
>> context of another process): do_notify_parent, pdeath_signal, f_setown,
>> and mq_notify.
>
> To me they all differ, I am not sure I understand how exactly you want
> to unify them...

The common thread is they all are requested by the receiver of the
signal (so don't need permission checks) and thus need to be canceled by
appropriate versions of exec.


>> Especially with the concerns about being able to send a signal after
>> exec, and cause havoc.
> ...
>> Espeically
>> with concerns about being able to send signals to a suid process that
>> would normally fail I think there is an issue here.
>
> I can easily misread this code, never looked into ipc/mqueue.c before.
> But it seems that it is not possible to send a signal after exec, suid
> or not,
>
> 	- sys_mq_open() uses O_CLOEXEC
>
> 	- mqueue_flush_file() does
> 	
> 		if (task_tgid(current) == info->notify_owner)
> 			remove_notification(info);

That is weird.  It looks like we are attempt to handle file descriptor
passing.  The unix98 description of exec says all mq file descriptors
shall be closed, but I can't find a word about file descriptor passing
with af_unix sockets.

>> At the very least can you add a big fat comment about the semantics
>> that userspace expects in this case?
>
> Me? You are kidding ;)
>
> I know absolutely nothing about ipc/mqueue, and when I read this code
> or manpage I find the semantics of mq_notify is very strange.

Well at least a small comment please that says the code started
performing the permission check and userspace code regressed.

Perhaps with the description of the userspace code in the commit log.
Perhaps a test case?

While someone knows what broke I would very much like to capture as much
detail as we can avoid breaking things again in the future.  If we don't
do that now we risk breaking something the next time that code is
changed.

My old copy of unix98 shows no permission checking, and permission
checking does not make sense to me.  So I think it is very much a
problem that I added permission checking by accident.

I really just want to be certain that things are fixed well enough that
we don't risk a regressing again the next time someone touches the code.

Eric