Re: Fw: [PATCH 1/1] file capabilities: simplify signal check

[Harald Welte]

On Sun, Feb 24, 2008 at 09:09:31PM +0300, Oleg Nesterov wrote:
> I just have an almost off-topic (sorry ;) question. Do we really need
> kill_pid_info_as_uid() ? Harald Welte cc'ed.
> 
> From "[PATCH] Fix signal sending in usbdevio on async URB completion"
> commit 46113830a18847cff8da73005e57bc49c2f95a56
> 
> 	> If a process issues an URB from userspace and (starts to) terminate
> 	> before the URB comes back, we run into the issue described above.  This
> 	> is because the urb saves a pointer to "current" when it is posted to the
> 	> device, but there's no guarantee that this pointer is still valid
> 	> afterwards.
> 	>
> 	> In fact, there are three separate issues:
> 	>
> 	> 1) the pointer to "current" can become invalid, since the task could be
> 	>    completely gone when the URB completion comes back from the device.
> 	>
> 	> 2) Even if the saved task pointer is still pointing to a valid task_struct,
> 	>    task_struct->sighand could have gone meanwhile.
> 	>
> 	> 3) Even if the process is perfectly fine, permissions may have changed,
> 	>    and we can no longer send it a signal.
> 
> The problems 1) and 2) are solved by converting to a struct pid. Is 3) a real
> problem? The task which does ioctl(USBDEVFS_SUBMITURB) explicitly asks to send
> the signal to it, should we deny the signal even if it changes its credentials
> in some way?

At the time I discovered the abovementioned problem, '1' and '2' were
real practical issues that I was seeing on live systems, triggerable
from userspace with no problems. '3' was more of a theoretical issue
that was discovered while reading the code and spending some thought on
it.

I personally am too remote to whatever you're currently doing to the
code ('using struct pid') in order to give any comment.  The overall
process of 'saving the current pointer and re-using it at some later
point while the original task might be gone or modified' must work.

Whether or not we should deny the signal even if the process changes its
own credentials in some way sounds like a much more esoteric question to
me.  I think it's fair to say that the resulting behavior is
"unspecified but shouldn't cause the process and/or kernel to misbehave"

At least I'm not aware of any usbdevio logic that would require some
specific behaviour here.

-- 
- Harald Welte <laforge@xxxxxxxxxxxx>           http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)

Attachment: signature.asc
Description: Digital signature