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

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 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 change
isn't necessary to this, but it just seemed a good time to add something
that feels like a hole in the paradigm.

Alexander: I've sort of put some words in your mouth, so feel free to
correct anything.

- Jamie