Date: Sun, 30 Mar 2014 08:04:35 +0800 From: David Xu <davidxu@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: Mateusz Guzik <mjguzik@gmail.com>, Mateusz Guzik <mjg@FreeBSD.org>, Don Lewis <truckman@FreeBSD.org>, svn-src-head@FreeBSD.org, src-committers@FreeBSD.org, kostikbel@gmail.com, svn-src-all@FreeBSD.org Subject: Re: svn commit: r263755 - head/sys/kern Message-ID: <53375F93.9070502@freebsd.org> In-Reply-To: <AA33BE58-3B3E-40D4-9FA2-541B7743B97D@gmail.com> References: <201403290752.s2T7qldY012467@gw.catspoiler.org> <5336BD22.1040906@freebsd.org> <AA33BE58-3B3E-40D4-9FA2-541B7743B97D@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2014/03/29 23:42, Warner Losh wrote: > On Mar 29, 2014, at 6:31 AM, David Xu <davidxu@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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?53375F93.9070502>