svn commit: r263755 - head/sys/kern

[David Xu]

On 2014/03/29 23:42, Warner Losh wrote:
> On Mar 29, 2014, at 6:31 AM, David Xu <davidxu at FreeBSD.org> wrote:
>
>> On 2014/03/29 15:52, Don Lewis wrote:
>>> On 29 Mar, Mateusz Guzik wrote:
>>>> On Sat, Mar 29, 2014 at 11:52:09AM +0800, David Xu wrote:
>>>>>> If fsetown handling like this is insecure this would bite us in that
>>>>>> scenario (and few others). In short, if we can avoid giving another way
>>>>>> to corrupt stuff in the kernel to userspace, we should.
>>>>>>
>>>>> I can not see what you said, where is the security problem with fsetown ?
>>>>> if you have per-jail instance of devsoftc, they all are operating on their
>>>>> own instance. but I don't think this patch should address jail now, there
>>>>> are many things are not jail ready.
>>>>>
>>>> I asked if multpiple concurrent calls to fsetown(.., &pointer) could
>>>> result in some corruption in the kernel - if they could, we would have a
>>>> problem in the future.
>>>>
>>>> I decided to read the code once more and fsetown seems to be safe in
>>>> this regard after all and with that in mind the patch looks good to me.
>>>   
>>> The fsetown() implementation does sufficient locking to prevent the
>>> kernel from getting into a bad state.  The issue is that the device can
>>> only have at most one owner (which may be a process group).  If multiple
>>> processes are allowed to open the device, or if a process that opened
>>> the device shares the descriptor with another process, the last call to
>>> fsetown() wins.  That means that one process could steal ownership from
>>> another if they both have the same device open.
>>>
>>> The reason that I suggested checking ownership when handling FIOASYNC is
>>> that in the case of two processes sharing access to a device, there is
>>> currently nothing that prevents a non-owner of the device from enabling
>>> this mode and causing SIGIO signals to be sent to the owner, which might
>>> not be expecting to receive them.
>> I think if you add ownership checking, it will be incompatible with
>> other code,  people have to change their mind when dealing with
>> this special file descriptor, my recommendation is people don't need
>> to refresh their brain.
>> OTOH, if it is a problem, we should have already been flooded by
>> the problem, but in the past years, I saw zero complaining in the mailing
>> lists.
> I believe that the SIGIO code was cut and pasted from a driver I was working
> on in the 4.x time frame. devd is the only consumer, and it doesn’t do the
> FIOASYNC stuff at all.
>
> So I’d be strongly biased to either (a) remove support for this or (b) make
> the support correct, even at the cost of speed or performance.
>
> Warner
Even with my patch, I think there is a race condition,  suppose owner 
was set,
and now I turn on FIOASYNC, should SIGIO signal be sent now if input or
output becomes possible ? if it is true, I found the bug in several piece of
kernel code, socket, pipe etcs are all infected.