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

[Oleg Nesterov]

On 02/24, Harald Welte wrote:
>
> 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.

Yes, yes, I see, the patch was fine.

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

OK, thanks.

So, the only reason why we can't kill kill_pid_info_as_uid() and just use
kill_pid_info() is that USB uses .si_code = SI_ASYNCIO. The latter means
that SI_FROMUSER(info) == T.

I assume that it is not an option to change USB to use .si_code > 0, yes?

Oleg.

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