Date: Sat, 25 May 2013 22:07:10 +0200 From: Alexander Leidinger <Alexander@Leidinger.net> To: Jamie Gritton <jamie@FreeBSD.org> Cc: Konstantin Belousov <kostikbel@gmail.com>, FreeBSD Current <freebsd-current@FreeBSD.org> Subject: Re: A PRIV_* flag for /dev/mem? Message-ID: <20130525220710.000041b5@unknown> In-Reply-To: <519783C1.2010601@FreeBSD.org> References: <5196818F.8080201@FreeBSD.org> <20130518114357.GK3047@kib.kiev.ua> <519783C1.2010601@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 18 May 2013 07:36:01 -0600 Jamie Gritton <jamie@FreeBSD.org> wrote: > On 05/18/13 05:43, Konstantin Belousov wrote: > > On Fri, May 17, 2013 at 01:14:23PM -0600, Jamie Gritton wrote: > >> I'm considering Alexander Leidinger's patch to make X11 work > >> inside a jail > >> (http://leidinger.net/FreeBSD/current-patches/0_jail.diff). It > >> allows a jail to optionally have access to /dev/io and DRI > >> (provided the requisite device files are visible in the devfs > >> ruleset). > >> > >> I'm planning on putting this under a single jail permission, which > >> would group those two together as device access that allows > >> messing with kernel memory. It seems more complete to > >> put /dev/mem under that same umbrella, with the side benefit of > >> letting me call it "allow.dev_mem". > >> > >> Currently, access is controlled only by device file permission and > >> a securelevel check. Jail access is allowed as long as > >> the /dev/mem is in the jail's ruleset (it isn't by default). > >> Adding a prison_priv_check() call would allow some finer control > >> over this. Something like: > >> > >> int > >> memopen(struct cdev *dev __unused, int flags, int fmt __unused, > >> struct thread *td) > >> { > >> int error; > >> > >> error = priv_check(td, PRIV_FOO); > >> if (error != 0&& (flags& FWRITE)) > >> error = securelevel_gt(td->td_ucred, 0); > >> > >> return (error); > >> } > >> > >> The main question I'm coming up with here is, what PRIV_* flag > >> should I use. Does PRIV_IO make sense? PRIV_DRIVER? Something > >> new like PRIV_KMEM? Also, I'd appreciate if anyone familiar with > >> this interface can tell me if memopen() is the right/only place to > >> make this change. > > > > Why do we need the PRIV check there at all, esp. for DRM ? > > Why the devfs rulesets are not enough ? > > At least for the reason Alexander's patch was first made, X11 working > inside a jail, there's already a need to get past PRIV_IO and > PRIV_DRIVER - those checks are already made so in that case the > presence of device files isn't sufficient. His solution was to > special-case PRIV_DRIVER for drm, and then add jail permission bits > that allowed PRIV_IO and PRIV_DRI_DRIVER. A largish but apparently > arbitrary set of of devices use PRIV_DRIVER, so it makes sense to > separate out this one that's necessary. > > So while there may be a question as to why /dev/io and DRM should have > PRIV checks, the fact of the matter is they already do. > > Now as to the change I'm considering: kmem. Since the main danger of > the existing checks (io and drm) is that they can allow you to stomp > on kernel memory, I thought it reasonable to combine them into a > single jail flag that allowed that behavior. In coming up with a > succinct name for it, I decided on allow.dev_mem (permission for > devices that work with system memory), and that brought up the > question for /dev/mem. No, I don't need to add a priv check to it; > but it seems that if such checks as PRIV_IO and PRIV_DRIVER exist for Info: I spoke with the author of the dri1 driver loooong ago, and it was OK for him if I would change the PRIV_DRIVER in DRI to something else. > devices already, then an architectural decision has already been made > that device file access isn't the only level of control we'd like to > have. Really I'm surprised something as potentially damaging as kmem > didn't already have a priv_check associated with it. > > Now I could certainly add his patch with no changes (or with very > few), and just put in a jail flag that's X11-specific. The /dev/mem I wouldn't be happy if my patch is committed as is. Your suggestion sounds much better. I would suggest "allow.kmem" or "allow.kmem_devs". The reason is that "dev_mem" could be seen as "/dev/mem" only. > change isn't necessary to this, but it just seemed a good time to add > something that feels like a hole in the paradigm. Bye, Alexander. -- http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID = B0063FE7 http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID = 72077137
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130525220710.000041b5>