From: Roland Dreier <[email protected]>
Commit da48524eb206 ("Prevent rt_sigqueueinfo and rt_tgsigqueueinfo
from spoofing the signal code") made the check on si_code too strict.
There are several legitimate places where glibc wants to queue a
negative si_code different from SI_QUEUE:
- This was first noticed with glibc's aio implementation, which wants
to queue a signal with si_code SI_ASYNCIO; the current kernel
causes glibc's tst-aio4 test to fail because rt_sigqueueinfo()
fails with EPERM.
- Further examination of the glibc source shows that getaddrinfo_a()
wants to use SI_ASYNCNL (which the kernel does not even define).
The timer_create() fallback code wants to queue signals with SI_TIMER.
As suggested by Oleg Nesterov <[email protected]>, loosen the check to
forbid only the problematic SI_TKILL case.
Reported-by: Klaus Dittrich <[email protected]>
Cc: <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
---
kernel/signal.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/signal.c b/kernel/signal.c
index 324eff5..b2bfa3a 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2437,7 +2437,7 @@ SYSCALL_DEFINE3(rt_sigqueueinfo, pid_t, pid, int, sig,
/* Not even root can pretend to send signals from the kernel.
* Nor can they impersonate a kill()/tgkill(), which adds source info.
*/
- if (info.si_code != SI_QUEUE) {
+ if (info.si_code >= 0 || info.si_code == SI_TKILL) {
/* We used to allow any < 0 si_code */
WARN_ON_ONCE(info.si_code < 0);
return -EPERM;
@@ -2457,7 +2457,7 @@ long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info)
/* Not even root can pretend to send signals from the kernel.
* Nor can they impersonate a kill()/tgkill(), which adds source info.
*/
- if (info->si_code != SI_QUEUE) {
+ if (info->si_code >= 0 || info->si_code == SI_TKILL) {
/* We used to allow any < 0 si_code */
WARN_ON_ONCE(info->si_code < 0);
return -EPERM;
Ohh, good catch!
It's not ideal that we have to relax this check, but as long as kernel
codes, SI_USER and SI_TKILL are not spoofable, I think we're in ok
shape.
Thanks,
Julien
On Mon, Mar 28, 2011 at 2:13 PM, Roland Dreier <[email protected]> wrote:
> From: Roland Dreier <[email protected]>
>
> Commit da48524eb206 ("Prevent rt_sigqueueinfo and rt_tgsigqueueinfo
> from spoofing the signal code") made the check on si_code too strict.
> There are several legitimate places where glibc wants to queue a
> negative si_code different from SI_QUEUE:
>
> ?- This was first noticed with glibc's aio implementation, which wants
> ? to queue a signal with si_code SI_ASYNCIO; the current kernel
> ? causes glibc's tst-aio4 test to fail because rt_sigqueueinfo()
> ? fails with EPERM.
>
> ?- Further examination of the glibc source shows that getaddrinfo_a()
> ? wants to use SI_ASYNCNL (which the kernel does not even define).
> ? The timer_create() fallback code wants to queue signals with SI_TIMER.
>
> As suggested by Oleg Nesterov <[email protected]>, loosen the check to
> forbid only the problematic SI_TKILL case.
>
> Reported-by: Klaus Dittrich <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Roland Dreier <[email protected]>
> ---
> ?kernel/signal.c | ? ?4 ++--
> ?1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 324eff5..b2bfa3a 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2437,7 +2437,7 @@ SYSCALL_DEFINE3(rt_sigqueueinfo, pid_t, pid, int, sig,
> ? ? ? ?/* Not even root can pretend to send signals from the kernel.
> ? ? ? ? * Nor can they impersonate a kill()/tgkill(), which adds source info.
> ? ? ? ? */
> - ? ? ? if (info.si_code != SI_QUEUE) {
> + ? ? ? if (info.si_code >= 0 || info.si_code == SI_TKILL) {
> ? ? ? ? ? ? ? ?/* We used to allow any < 0 si_code */
> ? ? ? ? ? ? ? ?WARN_ON_ONCE(info.si_code < 0);
> ? ? ? ? ? ? ? ?return -EPERM;
> @@ -2457,7 +2457,7 @@ long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info)
> ? ? ? ?/* Not even root can pretend to send signals from the kernel.
> ? ? ? ? * Nor can they impersonate a kill()/tgkill(), which adds source info.
> ? ? ? ? */
> - ? ? ? if (info->si_code != SI_QUEUE) {
> + ? ? ? if (info->si_code >= 0 || info->si_code == SI_TKILL) {
> ? ? ? ? ? ? ? ?/* We used to allow any < 0 si_code */
> ? ? ? ? ? ? ? ?WARN_ON_ONCE(info->si_code < 0);
> ? ? ? ? ? ? ? ?return -EPERM;
>
On 03/28, Roland Dreier wrote:
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 324eff5..b2bfa3a 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2437,7 +2437,7 @@ SYSCALL_DEFINE3(rt_sigqueueinfo, pid_t, pid, int, sig,
> /* Not even root can pretend to send signals from the kernel.
> * Nor can they impersonate a kill()/tgkill(), which adds source info.
> */
> - if (info.si_code != SI_QUEUE) {
> + if (info.si_code >= 0 || info.si_code == SI_TKILL) {
> /* We used to allow any < 0 si_code */
> WARN_ON_ONCE(info.si_code < 0);
This is equivalent to WARN_ON_ONCE(SI_TKILL)... doesn't matter, please ignore.
Thanks, I think -stable needs this patch asap. It turns out 2.6.32.36, 2.6.33.9,
2.6.37.6 and 2.6.38.2 pulled da48524eb20662618854bb3df2db01fc65f3070c.
If this change breaks something too, we can make even more conservative check.
Acked-by: Oleg Nesterov <[email protected]>
On Tue, Mar 29, 2011 at 5:19 AM, Oleg Nesterov <[email protected]> wrote:
> Thanks, I think -stable needs this patch asap. It turns out 2.6.32.36, 2.6.33.9,
> 2.6.37.6 and 2.6.38.2 pulled da48524eb20662618854bb3df2db01fc65f3070c.
Agree... the commit in Linus's tree is tagged for stable@.
The only issue I think is that Greg said he's not doing any more
2.6.37 releases?
Greg, this bug breaks the glibc test suite so maybe it's worth doing one more
release?
On Tue, Mar 29, 2011 at 10:26:55AM -0700, Roland Dreier wrote:
> On Tue, Mar 29, 2011 at 5:19 AM, Oleg Nesterov <[email protected]> wrote:
> > Thanks, I think -stable needs this patch asap. It turns out 2.6.32.36, 2.6.33.9,
> > 2.6.37.6 and 2.6.38.2 pulled da48524eb20662618854bb3df2db01fc65f3070c.
>
> Agree... the commit in Linus's tree is tagged for stable@.
>
> The only issue I think is that Greg said he's not doing any more
> 2.6.37 releases?
>
> Greg, this bug breaks the glibc test suite so maybe it's worth doing one more
> release?
Hm, I just checked the distros that I know of that are using the .37
kernel and they already have this patch in them, so it's not really
worth it to me to do another spin of the .37-stable tree. Especially as
I'd like to have everyone move on to .38-stable instead.
thanks,
greg k-h