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

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

[Oleg Nesterov]

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().

This needs the additional notify.sigev_signo != 0 check, shouldn't we
change do_mq_notify() to deny sigev_signo == 0 ?

Reported-by: Yoji <yoji.fujihar.min@gmail.com>
Fixes: cc731525f26a ("signal: Remove kernel interal si_code magic")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 ipc/mqueue.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 49a05ba3000d..3145fae162c1 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -775,12 +775,15 @@ static void __do_notify(struct mqueue_inode_info *info)
 	if (info->notify_owner &&
 	    info->attr.mq_curmsgs == 1) {
 		struct kernel_siginfo sig_i;
+		struct task_struct *task;
 		switch (info->notify.sigev_notify) {
 		case SIGEV_NONE:
 			break;
 		case SIGEV_SIGNAL:
+			/* do_mq_notify() accepts sigev_signo == 0, why?? */
+			if (!info->notify.sigev_signo)
+				break;
 			/* sends signal */
-
 			clear_siginfo(&sig_i);
 			sig_i.si_signo = info->notify.sigev_signo;
 			sig_i.si_errno = 0;
@@ -790,11 +793,13 @@ static void __do_notify(struct mqueue_inode_info *info)
 			rcu_read_lock();
 			sig_i.si_pid = task_tgid_nr_ns(current,
 						ns_of_pid(info->notify_owner));
-			sig_i.si_uid = from_kuid_munged(info->notify_user_ns, current_uid());
+			sig_i.si_uid = from_kuid_munged(info->notify_user_ns,
+						current_uid());
+			task = pid_task(info->notify_owner, PIDTYPE_PID);
+			if (task)
+				do_send_sig_info(info->notify.sigev_signo,
+						&sig_i, task, PIDTYPE_TGID);
 			rcu_read_unlock();
-
-			kill_pid_info(info->notify.sigev_signo,
-				      &sig_i, info->notify_owner);
 			break;
 		case SIGEV_THREAD:
 			set_cookie(info->notify_cookie, NOTIFY_WOKENUP);
-- 
2.25.1.362.g51ebf55

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

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> 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.  The code needs to make certain that this signal is
canceled or otherwise won't be sent after an exec.

That said I don't like it.  I would really like to remove the signal
sending interfaces that take a task_struct.

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.

When 4 different pieces of code are doing effectively the same thing and
have very similar if not the exact same concerns and they are all doing
things differently then we have a maintenance problem.

Especially with the concerns about being able to send a signal after
exec, and cause havoc.

Oleg is there any chance you can see if you can find a common helper
or a common idiom that all three cases can and should use?  Espeically
with concerns about being able to send signals to a suid process that
would normally fail I think there is an issue here.

At the very least can you add a big fat comment about the semantics
that userspace expects in this case?

> This needs the additional notify.sigev_signo != 0 check, shouldn't we
> change do_mq_notify() to deny sigev_signo == 0 ?

I wonder if the author of the code simply did not realize that
valid_signal allows 0.  As this is a posix interface we should be able
to check the posix spec and see if it gives any useful guidance about
signal 0.

Eric



> Reported-by: Yoji <yoji.fujihar.min@gmail.com>
> Fixes: cc731525f26a ("signal: Remove kernel interal si_code magic")
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  ipc/mqueue.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 49a05ba3000d..3145fae162c1 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -775,12 +775,15 @@ static void __do_notify(struct mqueue_inode_info *info)
>  	if (info->notify_owner &&
>  	    info->attr.mq_curmsgs == 1) {
>  		struct kernel_siginfo sig_i;
> +		struct task_struct *task;
>  		switch (info->notify.sigev_notify) {
>  		case SIGEV_NONE:
>  			break;
>  		case SIGEV_SIGNAL:
> +			/* do_mq_notify() accepts sigev_signo == 0, why?? */
> +			if (!info->notify.sigev_signo)
> +				break;
>  			/* sends signal */
> -
>  			clear_siginfo(&sig_i);
>  			sig_i.si_signo = info->notify.sigev_signo;
>  			sig_i.si_errno = 0;
> @@ -790,11 +793,13 @@ static void __do_notify(struct mqueue_inode_info *info)
>  			rcu_read_lock();
>  			sig_i.si_pid = task_tgid_nr_ns(current,
>  						ns_of_pid(info->notify_owner));
> -			sig_i.si_uid = from_kuid_munged(info->notify_user_ns, current_uid());
> +			sig_i.si_uid = from_kuid_munged(info->notify_user_ns,
> +						current_uid());
> +			task = pid_task(info->notify_owner, PIDTYPE_PID);
> +			if (task)
> +				do_send_sig_info(info->notify.sigev_signo,
> +						&sig_i, task, PIDTYPE_TGID);
>  			rcu_read_unlock();
> -
> -			kill_pid_info(info->notify.sigev_signo,
> -				      &sig_i, info->notify_owner);
>  			break;
>  		case SIGEV_THREAD:
>  			set_cookie(info->notify_cookie, NOTIFY_WOKENUP);

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

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> Oleg Nesterov <oleg@redhat.com> 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.  The code needs to make certain that this signal is
> canceled or otherwise won't be sent after an exec.
>
> That said I don't like it.  I would really like to remove the signal
> sending interfaces that take a task_struct.
>
> 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.

Scratch the fctnl(F_SETOWN,...) case for sending SIGIO.  While
frequently that applies to sending to yourself it isn't required and
that path does have a permission check.  So that whole case is clearly
something different.

That still leaves us with at least 3 very similar cases in the kernel.

Eric


> When 4 different pieces of code are doing effectively the same thing and
> have very similar if not the exact same concerns and they are all doing
> things differently then we have a maintenance problem.
>
> Especially with the concerns about being able to send a signal after
> exec, and cause havoc.
>
> Oleg is there any chance you can see if you can find a common helper
> or a common idiom that all three cases can and should use?  Espeically
> with concerns about being able to send signals to a suid process that
> would normally fail I think there is an issue here.
>
> At the very least can you add a big fat comment about the semantics
> that userspace expects in this case?
>
>> This needs the additional notify.sigev_signo != 0 check, shouldn't we
>> change do_mq_notify() to deny sigev_signo == 0 ?
>
> I wonder if the author of the code simply did not realize that
> valid_signal allows 0.  As this is a posix interface we should be able
> to check the posix spec and see if it gives any useful guidance about
> signal 0.
>
> Eric
>
>
>
>> Reported-by: Yoji <yoji.fujihar.min@gmail.com>
>> Fixes: cc731525f26a ("signal: Remove kernel interal si_code magic")
>> Cc: stable <stable@vger.kernel.org>
>> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>> ---
>>  ipc/mqueue.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
>> index 49a05ba3000d..3145fae162c1 100644
>> --- a/ipc/mqueue.c
>> +++ b/ipc/mqueue.c
>> @@ -775,12 +775,15 @@ static void __do_notify(struct mqueue_inode_info *info)
>>  	if (info->notify_owner &&
>>  	    info->attr.mq_curmsgs == 1) {
>>  		struct kernel_siginfo sig_i;
>> +		struct task_struct *task;
>>  		switch (info->notify.sigev_notify) {
>>  		case SIGEV_NONE:
>>  			break;
>>  		case SIGEV_SIGNAL:
>> +			/* do_mq_notify() accepts sigev_signo == 0, why?? */
>> +			if (!info->notify.sigev_signo)
>> +				break;
>>  			/* sends signal */
>> -
>>  			clear_siginfo(&sig_i);
>>  			sig_i.si_signo = info->notify.sigev_signo;
>>  			sig_i.si_errno = 0;
>> @@ -790,11 +793,13 @@ static void __do_notify(struct mqueue_inode_info *info)
>>  			rcu_read_lock();
>>  			sig_i.si_pid = task_tgid_nr_ns(current,
>>  						ns_of_pid(info->notify_owner));
>> -			sig_i.si_uid = from_kuid_munged(info->notify_user_ns, current_uid());
>> +			sig_i.si_uid = from_kuid_munged(info->notify_user_ns,
>> +						current_uid());
>> +			task = pid_task(info->notify_owner, PIDTYPE_PID);
>> +			if (task)
>> +				do_send_sig_info(info->notify.sigev_signo,
>> +						&sig_i, task, PIDTYPE_TGID);
>>  			rcu_read_unlock();
>> -
>> -			kill_pid_info(info->notify.sigev_signo,
>> -				      &sig_i, info->notify_owner);
>>  			break;
>>  		case SIGEV_THREAD:
>>  			set_cookie(info->notify_cookie, NOTIFY_WOKENUP);

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

[Oleg Nesterov]

On 03/22, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> 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.

> 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.

> 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...

> 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);

> 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.

Oleg.

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

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 03/22, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> 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

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

[Andrew Morton]

On Mon, 23 Mar 2020 11:47:12 -0500 ebiederm@xmission.com (Eric W. Biederman) wrote:

> 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.

That would be nice ;)

But as Oleg indicated, please let's have something minimal for -stable
backporting friendliness.  A more comprehensive change can then be
merged following the regular processes.

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

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Mon, 23 Mar 2020 11:47:12 -0500 ebiederm@xmission.com (Eric W. Biederman) wrote:
>
>> 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.
>
> That would be nice ;)
>
> But as Oleg indicated, please let's have something minimal for -stable
> backporting friendliness.  A more comprehensive change can then be
> merged following the regular processes.

So far what we have is a report Oleg has read somewhere that some
program doing something regressed, and his patch to fix that specific
program.  This problem was not noticed for several years.

Presumably the problem is that a message queue was written to by one
user and was read by another user to cause check_kill_permission to
fail. Can someone tell me if that was the case?

At this point all we have are my vague hand wavy readings of the unix98
that even says not checking permissions is correct.

I could reheat the silly arguments I have seen around pdeath_signal and
why pdeath_signal needs a permission check to say that this mq_notify
also needs a permission check to prevent signaling a processes we should
not be able to signal.

So I am looking for something that makes it clear we are not removing
a permission checking and backporting a security hole.

Further even if in the common case it is the right thing to do to remove
the permission check, the handling around exec looks bad enough that we
will be backporting a security hole if we don't fix that and backport
that at the same time.

Eric

p.s. I am grouchy as temporary fixes in this part of the code base
     don't tend to be temporary  and the entire signal/exec/ptrace world
     is bordering on unmaintainble and incomprehensible as a result.

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

[Oleg Nesterov]

On 03/23, Eric W. Biederman wrote:
>
> So far what we have is a report Oleg has read somewhere that some
> program doing something regressed, and his patch to fix that specific
> program.  This problem was not noticed for several years.

Yes, this was reported on bugzilla.redhat.com, I'll add you to CC list.

> Presumably the problem is that a message queue was written to by one
> user and was read by another user to cause check_kill_permission to
> fail. Can someone tell me if that was the case?

I do not know. Yoji, did you hit this bug or did you find it by code
inspection ?

> So I am looking for something that makes it clear we are not removing
> a permission checking and backporting a security hole.

Yes, I thought about this too. I can be easily wrong, please correct me,
but I came to conclusion the old behaviour (no permission check) is fine
security-wise.

> Further even if in the common case it is the right thing to do to remove
> the permission check, the handling around exec looks bad enough that we
> will be backporting a security hole if we don't fix that and backport
> that at the same time.

could you explain what exactly you do not like wrt mq_notify/exec ?
I must have missed something.

> p.s. I am grouchy as temporary fixes in this part of the code base
>      don't tend to be temporary  and the entire signal/exec/ptrace world
>      is bordering on unmaintainble and incomprehensible as a result.

Eric, please feel free to make another fix you like more. I know that
I can't convince you anyway, I won't argue.
Oleg.

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

[Oleg Nesterov]

Meanwhile, let me send V2. I added the comment and updated the changelog.

On 03/24, Oleg Nesterov wrote:
>
> On 03/23, Eric W. Biederman wrote:
> >
> > So far what we have is a report Oleg has read somewhere that some
> > program doing something regressed, and his patch to fix that specific
> > program.  This problem was not noticed for several years.
>
> Yes, this was reported on bugzilla.redhat.com, I'll add you to CC list.
>
> > Presumably the problem is that a message queue was written to by one
> > user and was read by another user to cause check_kill_permission to
> > fail. Can someone tell me if that was the case?
>
> I do not know. Yoji, did you hit this bug or did you find it by code
> inspection ?
>
> > So I am looking for something that makes it clear we are not removing
> > a permission checking and backporting a security hole.
>
> Yes, I thought about this too. I can be easily wrong, please correct me,
> but I came to conclusion the old behaviour (no permission check) is fine
> security-wise.
>
> > Further even if in the common case it is the right thing to do to remove
> > the permission check, the handling around exec looks bad enough that we
> > will be backporting a security hole if we don't fix that and backport
> > that at the same time.
>
> could you explain what exactly you do not like wrt mq_notify/exec ?
> I must have missed something.
>
> > p.s. I am grouchy as temporary fixes in this part of the code base
> >      don't tend to be temporary  and the entire signal/exec/ptrace world
> >      is bordering on unmaintainble and incomprehensible as a result.
>
> Eric, please feel free to make another fix you like more. I know that
> I can't convince you anyway, I won't argue.
> Oleg.

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

[Oleg Nesterov]

On 03/23, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > 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.

Can't we do this on top of this patch? I want a trivially backportable fix
for -stable.

And who else can use this helper? And what exactly it should do? Should it
be called with rcu lock held? Should it check sig != 0? Name?

> 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.

Hmm. How can mqueue use SIGQUEUE_PREALLOC/send_sigqueue ? The timer can
pre-allocate sigqueue because it won't be used again until dequeued by
its single target, otherwise it just increments overrun.

mqueue can't. Just suppose that the user of mq_notify() blocks this signal,
then another task does mq_notify() and then __do_notify() is called again.

> > 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.

I don't really agree, but I won't argue.

> >> 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.

Yes. Yet I think they all differ, in particular in how they handle the
exec case. So I still do not understand a) how you are going to unify this
logic and b) why should we do this _before_ the fix.

> > 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.

Not sure I understand how this connects to fd-passing...

What I tried to say is that mqueue_fd will be closed on exec. This is not
not enough, but mqueue_flush_file==mqueue_file_operations->flush called
by filp_close() will remove the notification to ensure the signal can't
be sent after that.

> > 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.

Ah, OK, agreed, I'll add a comment.

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

OK, how about

	static int notified;

	static void sigh(int sig)
	{
		notified = 1;
	}

	int main(void)
	{
		signal(SIGIO, sigh);

		int fd = mq_open("/mq", O_RDWR|O_CREAT, 0666, NULL);
		assert(fd >= 0);

		struct sigevent se = {
			.sigev_notify	= SIGEV_SIGNAL,
			.sigev_signo	= SIGIO,
		};
		assert(mq_notify(fd, &se) == 0);

		if (!fork()) {
			setuid(1);
			mq_send(fd, "",1,0);
			return 0;
		}

		wait(NULL);
		mq_unlink("/mq");
		assert(notified);
		return 0;
	}

? Needs root.

Just in case... it passes on my machine running 4.2.8-300.fc23.x86_64, fails
with upstream kernel, the patch seems to work and looks very simple.

Oleg.

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

[Oleg Nesterov]

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().

This needs the additional notify.sigev_signo != 0 check, shouldn't we
change do_mq_notify() to deny sigev_signo == 0 ?

Test-case:

	#include <signal.h>
	#include <mqueue.h>
	#include <unistd.h>
	#include <sys/wait.h>
	#include <assert.h>

	static int notified;

	static void sigh(int sig)
	{
		notified = 1;
	}

	int main(void)
	{
		signal(SIGIO, sigh);

		int fd = mq_open("/mq", O_RDWR|O_CREAT, 0666, NULL);
		assert(fd >= 0);

		struct sigevent se = {
			.sigev_notify	= SIGEV_SIGNAL,
			.sigev_signo	= SIGIO,
		};
		assert(mq_notify(fd, &se) == 0);

		if (!fork()) {
			assert(setuid(1) == 0);
			mq_send(fd, "",1,0);
			return 0;
		}

		wait(NULL);
		mq_unlink("/mq");
		assert(notified);
		return 0;
	}

Reported-by: Yoji <yoji.fujihar.min@gmail.com>
Fixes: cc731525f26a ("signal: Remove kernel interal si_code magic")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 ipc/mqueue.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 49a05ba3000d..63b164932ffd 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -774,28 +774,40 @@ static void __do_notify(struct mqueue_inode_info *info)
 	 * synchronously. */
 	if (info->notify_owner &&
 	    info->attr.mq_curmsgs == 1) {
-		struct kernel_siginfo sig_i;
 		switch (info->notify.sigev_notify) {
 		case SIGEV_NONE:
 			break;
-		case SIGEV_SIGNAL:
-			/* sends signal */
+		case SIGEV_SIGNAL: {
+			struct kernel_siginfo sig_i;
+			struct task_struct *task;
+
+			/* do_mq_notify() accepts sigev_signo == 0, why?? */
+			if (!info->notify.sigev_signo)
+				break;
 
 			clear_siginfo(&sig_i);
 			sig_i.si_signo = info->notify.sigev_signo;
 			sig_i.si_errno = 0;
 			sig_i.si_code = SI_MESGQ;
 			sig_i.si_value = info->notify.sigev_value;
-			/* map current pid/uid into info->owner's namespaces */
 			rcu_read_lock();
+			/* map current pid/uid into info->owner's namespaces */
 			sig_i.si_pid = task_tgid_nr_ns(current,
 						ns_of_pid(info->notify_owner));
-			sig_i.si_uid = from_kuid_munged(info->notify_user_ns, current_uid());
+			sig_i.si_uid = from_kuid_munged(info->notify_user_ns,
+						current_uid());
+			/*
+			 * We can't use kill_pid_info(), this signal should
+			 * bypass check_kill_permission(). It is from kernel
+			 * but si_fromuser() can't know this.
+			 */
+			task = pid_task(info->notify_owner, PIDTYPE_PID);
+			if (task)
+				do_send_sig_info(info->notify.sigev_signo,
+						&sig_i, task, PIDTYPE_TGID);
 			rcu_read_unlock();
-
-			kill_pid_info(info->notify.sigev_signo,
-				      &sig_i, info->notify_owner);
 			break;
+		}
 		case SIGEV_THREAD:
 			set_cookie(info->notify_cookie, NOTIFY_WOKENUP);
 			netlink_sendskb(info->notify_sock, info->notify_cookie);
-- 
2.25.1.362.g51ebf55

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

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> 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().
>
> This needs the additional notify.sigev_signo != 0 check, shouldn't we
> change do_mq_notify() to deny sigev_signo == 0 ?
>
> Test-case:
>
> 	#include <signal.h>
> 	#include <mqueue.h>
> 	#include <unistd.h>
> 	#include <sys/wait.h>
> 	#include <assert.h>
>
> 	static int notified;
>
> 	static void sigh(int sig)
> 	{
> 		notified = 1;
> 	}
>
> 	int main(void)
> 	{
> 		signal(SIGIO, sigh);
>
> 		int fd = mq_open("/mq", O_RDWR|O_CREAT, 0666, NULL);
> 		assert(fd >= 0);
>
> 		struct sigevent se = {
> 			.sigev_notify	= SIGEV_SIGNAL,
> 			.sigev_signo	= SIGIO,
> 		};
> 		assert(mq_notify(fd, &se) == 0);
>
> 		if (!fork()) {
> 			assert(setuid(1) == 0);
> 			mq_send(fd, "",1,0);
> 			return 0;
> 		}
>
> 		wait(NULL);
> 		mq_unlink("/mq");
> 		assert(notified);
> 		return 0;
> 	}
>
> Reported-by: Yoji <yoji.fujihar.min@gmail.com>
> Fixes: cc731525f26a ("signal: Remove kernel interal si_code magic")
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  ipc/mqueue.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 49a05ba3000d..63b164932ffd 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -774,28 +774,40 @@ static void __do_notify(struct mqueue_inode_info *info)
>  	 * synchronously. */
>  	if (info->notify_owner &&
>  	    info->attr.mq_curmsgs == 1) {
> -		struct kernel_siginfo sig_i;
>  		switch (info->notify.sigev_notify) {
>  		case SIGEV_NONE:
>  			break;
> -		case SIGEV_SIGNAL:
> -			/* sends signal */
> +		case SIGEV_SIGNAL: {
> +			struct kernel_siginfo sig_i;
> +			struct task_struct *task;
> +
> +			/* do_mq_notify() accepts sigev_signo == 0, why?? */
> +			if (!info->notify.sigev_signo)
> +				break;
>  
>  			clear_siginfo(&sig_i);
>  			sig_i.si_signo = info->notify.sigev_signo;
>  			sig_i.si_errno = 0;
>  			sig_i.si_code = SI_MESGQ;
>  			sig_i.si_value = info->notify.sigev_value;
> -			/* map current pid/uid into info->owner's namespaces */
>  			rcu_read_lock();
> +			/* map current pid/uid into info->owner's namespaces */
>  			sig_i.si_pid = task_tgid_nr_ns(current,
>  						ns_of_pid(info->notify_owner));
> -			sig_i.si_uid = from_kuid_munged(info->notify_user_ns, current_uid());
> +			sig_i.si_uid = from_kuid_munged(info->notify_user_ns,
> +						current_uid());
> +			/*
> +			 * We can't use kill_pid_info(), this signal should
> +			 * bypass check_kill_permission(). It is from kernel
> +			 * but si_fromuser() can't know this.
> +			 */
> +			task = pid_task(info->notify_owner, PIDTYPE_PID);
                                                            ^^^^^^^^^^^^
Minor nit:  If we are doing the task lookup ourselves that can and
            should be PIDTYPE_TGID.  Because the code captures the TGID
            itself none of the very valid backwards compatibility
            reasons for using PIDTYPE_PID come into play.

> +			if (task)
> +				do_send_sig_info(info->notify.sigev_signo,
> +						&sig_i, task, PIDTYPE_TGID);
>  			rcu_read_unlock();
> -
> -			kill_pid_info(info->notify.sigev_signo,
> -				      &sig_i, info->notify_owner);
>  			break;
> +		}
>  		case SIGEV_THREAD:
>  			set_cookie(info->notify_cookie, NOTIFY_WOKENUP);
>  			netlink_sendskb(info->notify_sock, info->notify_cookie);
Eric

[PATCH -mm] ipc-mqueuec-change-__do_notify-to-bypass-check_kill_permission-fix

[Oleg Nesterov]

On 03/26, Eric W. Biederman wrote:
>
> > +			task = pid_task(info->notify_owner, PIDTYPE_PID);
>                                                             ^^^^^^^^^^^^
> Minor nit:  If we are doing the task lookup ourselves that can and
>             should be PIDTYPE_TGID.

I think this shouldn't make any difference, in particular because
do_mq_notify() does "notify_owner = task_tgid()" and we do not care
about exec.

But I agree, pid_task(PIDTYPE_TGID) looks better, thanks.


diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 63b164932ffd..9a44dcb04e13 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -801,7 +801,7 @@ static void __do_notify(struct mqueue_inode_info *info)
 			 * bypass check_kill_permission(). It is from kernel
 			 * but si_fromuser() can't know this.
 			 */
-			task = pid_task(info->notify_owner, PIDTYPE_PID);
+			task = pid_task(info->notify_owner, PIDTYPE_TGID);
 			if (task)
 				do_send_sig_info(info->notify.sigev_signo,
 						&sig_i, task, PIDTYPE_TGID);