linux-kernel - Re: [PATCH] ptrace: add ability to retrieve signals without removing from a queue (v2)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 26 Feb 2013 11:33:39 +0400
From:	Andrey Wagin <avagin@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org, criu@...nvz.org,
	linux-api@...r.kernel.org, Roland McGrath <roland@...hat.com>,
	Oleg Nesterov <oleg@...hat.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	David Howells <dhowells@...hat.com>,
	Dave Jones <davej@...hat.com>,
	"Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>,
	Pavel Emelyanov <xemul@...allels.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Pedro Alves <palves@...hat.com>
Subject: Re: [PATCH] ptrace: add ability to retrieve signals without removing
 from a queue (v2)

Andrew, thank you for the comments. I will fix them and send a new
patch. A few comments are inline.

2013/2/26 Andrew Morton <akpm@...ux-foundation.org>:
> On Mon, 25 Feb 2013 14:06:53 +0400
>> +     for (i = 0; i < arg.nr; i++) {
>> +             off = arg.off + i;
>
> "long long" = "u64" + "int".
>
> Wanna have a little think about what we're doing here, see if we can
> pick the most appropriate types?

A number of signal has the type int, because the system call ptrace()
return "int" and my interface supposed to return a number of dumped
signals.
"off" is signed to check on a negative value and it can not be more
then MAX_RAM / sizeof(siginfo), so
long long is enough. It can be changed on s64 for avoiding misleading.

>
> `off' could be made local to this loop, which is a bit neater.
>
>> +             spin_lock_irq(&child->sighand->siglock);
>> +             list_for_each_entry(q, &pending->list, list) {
>> +                     if (!off--) {
>> +                             copy_siginfo(&info, &q->info);
>> +                             break;
>> +                     }
>> +             }
>> +             spin_unlock_irq(&child->sighand->siglock);
>> +
>> +             if (off >= 0)
>> +                     break;
>
> <scratches head>
>
> Oh I see what you did there - the user requested an entry which is
> beyond the end of the list? A code comment would be nice.
>
> What's the return value in this case?  "i".  So how does userspace find
> out what happened?

The request PTRACE_PEEKSIGINFO returns a number of dumped signals. If
a signal with the specified sequence number doesn't exist, ptrace
returns 0.

>
> Please fully describe the interface so things such as this can be
> understood.
>
>> +#ifdef CONFIG_COMPAT
>> +             if (unlikely(is_compat_task())) {
>> +                     compat_siginfo_t __user *uinfo = compat_ptr(data);
>> +
>> +                     ret = copy_siginfo_to_user32(uinfo, &info);
>> +                     if (!ret)
>> +                             ret = __put_user(info.si_code, &uinfo->si_code);
>> +             } else
>> +#endif
>> +             {
>> +                     siginfo_t __user *uinfo = (siginfo_t __user *) data;
>> +
>> +                     ret = copy_siginfo_to_user(uinfo, &info);
>> +                     if (!ret)
>> +                             ret = __put_user(info.si_code, &uinfo->si_code);
>> +             }
>> +
>> +             if (ret)
>> +                     break;
>> +
>> +             data += sizeof(siginfo_t);
>> +
>> +             if (signal_pending(current)) {
>> +                     i++;
>
> What does the i++ do?

For accounting a current siginfo. I will add a comment here.

>
>> +                     break;
>> +             }
>> +
>> +             cond_resched();
>> +     }
>> +
>> +     return i ? : ret;
>
> I hate that gcc thing :(
>
> Also, is it buggy?  `ret' might be -EFAULT here.  We appear to be using
> read() return semantics here?

I have tried to implement read() semantics here. ptrace() returns a
number of dumped signals even if dumping of the last signal is failed.
And it returns an error if not one signal has been dumped
successfully.

The similar logic is used in read()
generic_file_aio_read()
....
                retval += desc.written;
                if (desc.error) {
                        retval = retval ?: desc.error;
                        break;
                }

Thanks,
  Andrey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/