Alexander Leidinger wrote:
On Sat, 18 May 2013 07:36:01 -0600
Jamie Gritton <ja...@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.
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
