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

Alexander Leidinger wrote:
> 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.
> 

I have 2 comments on this subject.

If I understand correctly, all the names being suggested are for an 
internal flag name. What it's called internally does not interest me. 
But using the internal flag name as the jail(8) parameter name would be 
misleading and confusing. The single purpose of this patch is to enable 
xorg to run in a jail. Naming it after some internal nob that the patch 
tweaks makes no sense. Naming it "allow.xorg" identifies it's intended 
purpose in a user-friendly way and is crystal clear to every one no 
matter their level of technical knowledge.

Correct me if I am wrong here, but what this patch does internally 
breaks the security of the jail container. There are already jail(8) 
parameters that do this, so this is not new. I strongly suggest that the 
documentation on this new parameter contains strong wording that informs 
the reader of this security exposure and that it should NOT be used on a 
jail exposed to public internet access.