Date: Sat, 29 Mar 2014 09:42:08 -0600 From: Warner Losh <imp@bsdimp.com> To: David Xu <davidxu@FreeBSD.org> 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: <AA33BE58-3B3E-40D4-9FA2-541B7743B97D@gmail.com> In-Reply-To: <5336BD22.1040906@freebsd.org> References: <201403290752.s2T7qldY012467@gw.catspoiler.org> <5336BD22.1040906@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mar 29, 2014, at 6:31 AM, David Xu <davidxu@FreeBSD.org> wrote: >=20 > 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. >>>>>=20 >>>> 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. >>>>=20 >>> 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. >>>=20 >>> 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. >> =20 >> 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. >>=20 >> 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=92t do = the FIOASYNC stuff at all. So I=92d 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=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AA33BE58-3B3E-40D4-9FA2-541B7743B97D>