Re: [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO
On Tue, Jun 04, 2019 at 02:42:23PM -0500, Eric W. Biederman wrote:
> Andrei Vagin writes:
>
> > On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman
> > wrote:
> >>
> >>
> >> Recently syzbot in conjunction with KMSAN reported that
> >> ptrace_peek_siginfo can copy an uninitialized siginfo to userspace.
> >> Inspecting ptrace_peek_siginfo confirms this.
> >>
> >> The problem is that off when initialized from args.off can be
> >> initialized to a negaive value. At which point the "if (off >= 0)"
> >> test to see if off became negative fails because off started off
> >> negative.
> >>
> >> Prevent the core problem by adding a variable found that is only true
> >> if a siginfo is found and copied to a temporary in preparation for
> >> being copied to userspace.
> >>
> >> Prevent args.off from being truncated when being assigned to off by
> >> testing that off is <= the maximum possible value of off. Convert off
> >> to an unsigned long so that we should not have to truncate args.off,
> >> we have well defined overflow behavior so if we add another check we
> >> won't risk fighting undefined compiler behavior, and so that we have a
> >> type whose maximum value is easy to test for.
> >>
> >
> > Hello Eric,
> >
> > Thank you for fixing this issue. Sorry for the late response.
> > I thought it was fixed a few month ago, I remembered that we discussed it:
> > https://lkml.org/lkml/2018/10/10/251
>
> I was looking for that conversation, and I couldn't find it so I just
> decided to write a test and fix it.
>
> > Here are two inline comments.
> >
> >
> >> Cc: Andrei Vagin
> >> Cc: sta...@vger.kernel.org
> >> Reported-by: syzbot+0d602a1b0d8c95bdf...@syzkaller.appspotmail.com
> >> Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without
> >> removing from a queue (v4)")
> >> Signed-off-by: "Eric W. Biederman"
> >> ---
> >>
> >> Comments?
> >> Concerns?
> >>
> >> Otherwise I will queue this up and send it to Linus.
> >>
> >> kernel/ptrace.c | 10 --
> >> 1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> >> index 6f357f4fc859..4c2b24a885d3 100644
> >> --- a/kernel/ptrace.c
> >> +++ b/kernel/ptrace.c
> >> @@ -704,6 +704,10 @@ static int ptrace_peek_siginfo(struct task_struct
> >> *child,
> >> if (arg.nr < 0)
> >> return -EINVAL;
> >>
> >> + /* Ensure arg.off fits in an unsigned */
> >> + if (arg.off > ULONG_MAX)
> >
> > if (arg.off > ULONG_MAX - arg.nr)
> >
>
> The new variable found ensures that whatever we pass in we won't return
> an invalid value. All this test does is guarantee we don't return a
> much lower entry in the queue.
>
> We don't need to take arg.nr into account as we won't try
> entries that high as the queue will never get that long. The maximum
> siqueue entries per user is about 2^24.
>
> >> + return 0;
> >
> > maybe we should return EINVAL in this case
>
> But it is a huge request not an invalid request. The request
> makes perfect sense. For smaller values whose offset is
> greater than the length of the queue we just return 0 entries
> found. So I think it makes more sense to just return 0 entries
> found in this case as well.
>
> >> +
> >> if (arg.flags & PTRACE_PEEKSIGINFO_SHARED)
> >> pending = >signal->shared_pending;
> >> else
> >> @@ -711,18 +715,20 @@ static int ptrace_peek_siginfo(struct task_struct
> >> *child,
> >>
> >> for (i = 0; i < arg.nr; ) {
> >> kernel_siginfo_t info;
> >> - s32 off = arg.off + i;
> >> + unsigned long off = arg.off + i;
> >> + bool found = false;
> >>
> >> spin_lock_irq(>sighand->siglock);
> >> list_for_each_entry(q, >list, list) {
> >> if (!off--) {
> >> + found = true;
> >> copy_siginfo(, >info);
> >> break;
> >> }
> >> }
> >> spin_unlock_irq(>sighand->siglock);
> >>
> >> - if (off >= 0) /* beyond the end of the list */
> >> + if (!found) /* beyond the end of the list */
> >> break;
> >>
> >> #ifdef CONFIG_COMPAT
> >> --
> >> 2.21.0.dirty
> >>
>
This patch looks fine to me. Are you planning to queue this up?
It would be nice if we could fix this sort of bug in fewer than 8 months.
- Eric
Re: [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO
Andrei Vagin writes:
> On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman
> wrote:
>>
>>
>> Recently syzbot in conjunction with KMSAN reported that
>> ptrace_peek_siginfo can copy an uninitialized siginfo to userspace.
>> Inspecting ptrace_peek_siginfo confirms this.
>>
>> The problem is that off when initialized from args.off can be
>> initialized to a negaive value. At which point the "if (off >= 0)"
>> test to see if off became negative fails because off started off
>> negative.
>>
>> Prevent the core problem by adding a variable found that is only true
>> if a siginfo is found and copied to a temporary in preparation for
>> being copied to userspace.
>>
>> Prevent args.off from being truncated when being assigned to off by
>> testing that off is <= the maximum possible value of off. Convert off
>> to an unsigned long so that we should not have to truncate args.off,
>> we have well defined overflow behavior so if we add another check we
>> won't risk fighting undefined compiler behavior, and so that we have a
>> type whose maximum value is easy to test for.
>>
>
> Hello Eric,
>
> Thank you for fixing this issue. Sorry for the late response.
> I thought it was fixed a few month ago, I remembered that we discussed it:
> https://lkml.org/lkml/2018/10/10/251
I was looking for that conversation, and I couldn't find it so I just
decided to write a test and fix it.
> Here are two inline comments.
>
>
>> Cc: Andrei Vagin
>> Cc: sta...@vger.kernel.org
>> Reported-by: syzbot+0d602a1b0d8c95bdf...@syzkaller.appspotmail.com
>> Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without
>> removing from a queue (v4)")
>> Signed-off-by: "Eric W. Biederman"
>> ---
>>
>> Comments?
>> Concerns?
>>
>> Otherwise I will queue this up and send it to Linus.
>>
>> kernel/ptrace.c | 10 --
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
>> index 6f357f4fc859..4c2b24a885d3 100644
>> --- a/kernel/ptrace.c
>> +++ b/kernel/ptrace.c
>> @@ -704,6 +704,10 @@ static int ptrace_peek_siginfo(struct task_struct
>> *child,
>> if (arg.nr < 0)
>> return -EINVAL;
>>
>> + /* Ensure arg.off fits in an unsigned */
>> + if (arg.off > ULONG_MAX)
>
> if (arg.off > ULONG_MAX - arg.nr)
>
The new variable found ensures that whatever we pass in we won't return
an invalid value. All this test does is guarantee we don't return a
much lower entry in the queue.
We don't need to take arg.nr into account as we won't try
entries that high as the queue will never get that long. The maximum
siqueue entries per user is about 2^24.
>> + return 0;
>
> maybe we should return EINVAL in this case
But it is a huge request not an invalid request. The request
makes perfect sense. For smaller values whose offset is
greater than the length of the queue we just return 0 entries
found. So I think it makes more sense to just return 0 entries
found in this case as well.
>> +
>> if (arg.flags & PTRACE_PEEKSIGINFO_SHARED)
>> pending = >signal->shared_pending;
>> else
>> @@ -711,18 +715,20 @@ static int ptrace_peek_siginfo(struct task_struct
>> *child,
>>
>> for (i = 0; i < arg.nr; ) {
>> kernel_siginfo_t info;
>> - s32 off = arg.off + i;
>> + unsigned long off = arg.off + i;
>> + bool found = false;
>>
>> spin_lock_irq(>sighand->siglock);
>> list_for_each_entry(q, >list, list) {
>> if (!off--) {
>> + found = true;
>> copy_siginfo(, >info);
>> break;
>> }
>> }
>> spin_unlock_irq(>sighand->siglock);
>>
>> - if (off >= 0) /* beyond the end of the list */
>> + if (!found) /* beyond the end of the list */
>> break;
>>
>> #ifdef CONFIG_COMPAT
>> --
>> 2.21.0.dirty
>>
Eric
Re: [PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO
On Tue, May 28, 2019 at 6:22 PM Eric W. Biederman wrote:
>
>
> Recently syzbot in conjunction with KMSAN reported that
> ptrace_peek_siginfo can copy an uninitialized siginfo to userspace.
> Inspecting ptrace_peek_siginfo confirms this.
>
> The problem is that off when initialized from args.off can be
> initialized to a negaive value. At which point the "if (off >= 0)"
> test to see if off became negative fails because off started off
> negative.
>
> Prevent the core problem by adding a variable found that is only true
> if a siginfo is found and copied to a temporary in preparation for
> being copied to userspace.
>
> Prevent args.off from being truncated when being assigned to off by
> testing that off is <= the maximum possible value of off. Convert off
> to an unsigned long so that we should not have to truncate args.off,
> we have well defined overflow behavior so if we add another check we
> won't risk fighting undefined compiler behavior, and so that we have a
> type whose maximum value is easy to test for.
>
Hello Eric,
Thank you for fixing this issue. Sorry for the late response.
I thought it was fixed a few month ago, I remembered that we discussed it:
https://lkml.org/lkml/2018/10/10/251
Here are two inline comments.
> Cc: Andrei Vagin
> Cc: sta...@vger.kernel.org
> Reported-by: syzbot+0d602a1b0d8c95bdf...@syzkaller.appspotmail.com
> Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without
> removing from a queue (v4)")
> Signed-off-by: "Eric W. Biederman"
> ---
>
> Comments?
> Concerns?
>
> Otherwise I will queue this up and send it to Linus.
>
> kernel/ptrace.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 6f357f4fc859..4c2b24a885d3 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -704,6 +704,10 @@ static int ptrace_peek_siginfo(struct task_struct *child,
> if (arg.nr < 0)
> return -EINVAL;
>
> + /* Ensure arg.off fits in an unsigned */
> + if (arg.off > ULONG_MAX)
if (arg.off > ULONG_MAX - arg.nr)
> + return 0;
maybe we should return EINVAL in this case
> +
> if (arg.flags & PTRACE_PEEKSIGINFO_SHARED)
> pending = >signal->shared_pending;
> else
> @@ -711,18 +715,20 @@ static int ptrace_peek_siginfo(struct task_struct
> *child,
>
> for (i = 0; i < arg.nr; ) {
> kernel_siginfo_t info;
> - s32 off = arg.off + i;
> + unsigned long off = arg.off + i;
> + bool found = false;
>
> spin_lock_irq(>sighand->siglock);
> list_for_each_entry(q, >list, list) {
> if (!off--) {
> + found = true;
> copy_siginfo(, >info);
> break;
> }
> }
> spin_unlock_irq(>sighand->siglock);
>
> - if (off >= 0) /* beyond the end of the list */
> + if (!found) /* beyond the end of the list */
> break;
>
> #ifdef CONFIG_COMPAT
> --
> 2.21.0.dirty
>
[PATCH] signal/ptrace: Don't leak unitialized kernel memory with PTRACE_PEEK_SIGINFO
Recently syzbot in conjunction with KMSAN reported that
ptrace_peek_siginfo can copy an uninitialized siginfo to userspace.
Inspecting ptrace_peek_siginfo confirms this.
The problem is that off when initialized from args.off can be
initialized to a negaive value. At which point the "if (off >= 0)"
test to see if off became negative fails because off started off
negative.
Prevent the core problem by adding a variable found that is only true
if a siginfo is found and copied to a temporary in preparation for
being copied to userspace.
Prevent args.off from being truncated when being assigned to off by
testing that off is <= the maximum possible value of off. Convert off
to an unsigned long so that we should not have to truncate args.off,
we have well defined overflow behavior so if we add another check we
won't risk fighting undefined compiler behavior, and so that we have a
type whose maximum value is easy to test for.
Cc: Andrei Vagin
Cc: sta...@vger.kernel.org
Reported-by: syzbot+0d602a1b0d8c95bdf...@syzkaller.appspotmail.com
Fixes: 84c751bd4aeb ("ptrace: add ability to retrieve signals without removing
from a queue (v4)")
Signed-off-by: "Eric W. Biederman"
---
Comments?
Concerns?
Otherwise I will queue this up and send it to Linus.
kernel/ptrace.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 6f357f4fc859..4c2b24a885d3 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -704,6 +704,10 @@ static int ptrace_peek_siginfo(struct task_struct *child,
if (arg.nr < 0)
return -EINVAL;
+ /* Ensure arg.off fits in an unsigned */
+ if (arg.off > ULONG_MAX)
+ return 0;
+
if (arg.flags & PTRACE_PEEKSIGINFO_SHARED)
pending = >signal->shared_pending;
else
@@ -711,18 +715,20 @@ static int ptrace_peek_siginfo(struct task_struct *child,
for (i = 0; i < arg.nr; ) {
kernel_siginfo_t info;
- s32 off = arg.off + i;
+ unsigned long off = arg.off + i;
+ bool found = false;
spin_lock_irq(>sighand->siglock);
list_for_each_entry(q, >list, list) {
if (!off--) {
+ found = true;
copy_siginfo(, >info);
break;
}
}
spin_unlock_irq(>sighand->siglock);
- if (off >= 0) /* beyond the end of the list */
+ if (!found) /* beyond the end of the list */
break;
#ifdef CONFIG_COMPAT
--
2.21.0.dirty
