From owner-svn-src-all@FreeBSD.ORG Sun Mar 30 00:04:46 2014 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 5E54D310; Sun, 30 Mar 2014 00:04:46 +0000 (UTC) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 3D9EAB6E; Sun, 30 Mar 2014 00:04:46 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.8/8.14.8) with ESMTP id s2U04cDI065583; Sun, 30 Mar 2014 00:04:42 GMT (envelope-from davidxu@freebsd.org) Message-ID: <53375F93.9070502@freebsd.org> Date: Sun, 30 Mar 2014 08:04:35 +0800 From: David Xu User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Warner Losh Subject: Re: svn commit: r263755 - head/sys/kern References: <201403290752.s2T7qldY012467@gw.catspoiler.org> <5336BD22.1040906@freebsd.org> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Cc: Mateusz Guzik , Mateusz Guzik , Don Lewis , svn-src-head@FreeBSD.org, src-committers@FreeBSD.org, kostikbel@gmail.com, svn-src-all@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 30 Mar 2014 00:04:46 -0000 On 2014/03/29 23:42, Warner Losh wrote: > On Mar 29, 2014, at 6:31 AM, David Xu 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.