Re: A PRIV_* flag for /dev/mem?

On Sat, 18 May 2013 07:36:01 -0600
Jamie Gritton <jamie_at_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 _at_ Leidinger.net: PGP ID = B0063FE7
http://www.FreeBSD.org       netchild _at_ FreeBSD.org  : PGP ID = 72077137