A PRIV_* flag for /dev/mem?

[Jamie Gritton]

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.

- Jamie
_______________________________________________
freebsd...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-curre...@freebsd.org"

[Jamie Gritton]

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.

[Kirk McKusick]

I pointed Robert and Pawel at your discussion on creating a new
PRIV_KMEM and adding a check for it in memopen(). I am of the opinion
that this is a good idea, but I am hoping that one of Robert or Pawel
will comment since they are much more active in this area.

Kirk McKusick

[Alexander Leidinger]

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

[Joe]

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.

[Jamie Gritton]

The function of the proposed jail flag is to allow changes to
kernel-level memory. The current best use for this might be to run an
X11 server. Currently, the X11 server in favor is Xorg. So we should
call it allow.xorg? Good thing we didn't do it a few years ago, or it
would confusingly be called allow.xfree86. Perhaps some other X11 server
will fall into favor. Perhaps some other graphics system will become
more popular than the aging X11 (admittedly not likely). And someone may
have some other reason to have a jail that has kernel memory permission.

So, no. Changing the name from function to "purpose" is not at all clear.

> 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.

True. I don't know about "strong" wording, but it least it should be
mentioned.

- Jamie

[Jamie Gritton]

On 05/20/13 16:56, Kirk McKusick wrote:
> I pointed Robert and Pawel at your discussion on creating a new
> PRIV_KMEM and adding a check for it in memopen(). I am of the opinion
> that this is a good idea, but I am hoping that one of Robert or Pawel
> will comment since they are much more active in this area.

I suppose it's safe to say further comment isn't forthcoming. So with
one vote for and one against (or at least questioning), I'll humbly
leave it up to myself to be the tie-breaker :-).

Here's a proposed patch. I separate kmem access into read and write, as
I saw other similar splits in the priv list. Perhaps that's overkill,
and I can use a single PRIV_KMEM instead of PRIV_KMEM_READ and
PRIV_KMEM_WRITE.

Perhaps this is an overreach, because PRIV_KMEM_READ is used where the
default isn't root privilege: the file permission and expected usage are
group kmem gets to read /dev/[k]mem. I'm not about to go hard-coding a
gid into the kernel, so it seems the proper thing to do (not included in
the patch) would be to allow PRIV_KMEM_READ by default. I thought there
might already be such cases where the default is to allow, but no: this
would be the first default-allow permission. So perhaps the best answer
is not worry about that one, and only add PRIV_KMEM_WRITE (leaving reads
controlled by file permission alone as they are now).

- Jamie

[Jamie Gritton]

On 06/16/13 00:20, Konstantin Belousov wrote:
> On Sat, Jun 15, 2013 at 05:23:50PM -0600, Jamie Gritton wrote:
>> Index: sys/dev/mem/memdev.c
>> ===================================================================
>> --- sys/dev/mem/memdev.c (revision 251793)
>> +++ sys/dev/mem/memdev.c (working copy)
>> @@ -67,8 +67,14 @@
>> {
>> int error = 0;
>>
>> - if (flags& FWRITE)
>> - error = securelevel_gt(td->td_ucred, 0);
>> + if (flags& FREAD)
>> + error = priv_check(td, PRIV_KMEM_READ);
>> + if (flags& FWRITE) {
>> + if (error != 0)
>> + error = priv_check(td, PRIV_KMEM_WRITE);
>> + if (error != 0)
> Shouldn't this be 'if (error == 0)' ?

Indeed it should. None of this has even been compiled yet, let alone
tested - still in the thought phase.

- Jamie

[Kirk McKusick]

> Date: Sat, 15 Jun 2013 17:23:50 -0600
> From: Jamie Gritton <ja...@FreeBSD.org>
> To: FreeBSD Current <freebsd...@FreeBSD.org>
> CC: Kirk McKusick <mcku...@mckusick.com>,
> Konstantin Belousov <kost...@gmail.com>,
> Alexander Leidinger <netc...@FreeBSD.org>,
> Pawel Jakub Dawidek <p...@FreeBSD.org>,
> Robert Watson <rwa...@FreeBSD.org>
> Subject: Re: A PRIV_* flag for /dev/mem?

With the change from the error noted by Kostik, I concur with your
proposed change.

Kirk McKusick

[Robert N. M. Watson]

On 16 Jun 2013, at 23:48, Kirk McKusick wrote:

>> I suppose it's safe to say further comment isn't forthcoming. So with
>> one vote for and one against (or at least questioning), I'll humbly
>> leave it up to myself to be the tie-breaker :-).
>>
>> Here's a proposed patch. I separate kmem access into read and write, as
>> I saw other similar splits in the priv list. Perhaps that's overkill,
>> and I can use a single PRIV_KMEM instead of PRIV_KMEM_READ and
>> PRIV_KMEM_WRITE.
>>
>> Perhaps this is an overreach, because PRIV_KMEM_READ is used where the
>> default isn't root privilege: the file permission and expected usage are
>> group kmem gets to read /dev/[k]mem. I'm not about to go hard-coding a
>> gid into the kernel, so it seems the proper thing to do (not included in
>> the patch) would be to allow PRIV_KMEM_READ by default. I thought there
>> might already be such cases where the default is to allow, but no: this
>> would be the first default-allow permission. So perhaps the best answer
>> is not worry about that one, and only add PRIV_KMEM_WRITE (leaving reads
>> controlled by file permission alone as they are now).
>

> With the change from the error noted by Kostik, I concur with your
> proposed change.

On the whole, the kernel privilege system is fairly straight-forward; the one real exception its its interaction with device-driver specific interfaces that seem privilege-like. On the whole, I take the view that we should define privileges in device drivers where either (a) there's a securelevel interaction or (b) it's likely we'll want to delegate access to the device node (perhaps in jail, or via chmod/chown) but there are special operations we still want to require privilege. For example, you could imagine wanting read/write access to a disk to be delegated, but not control operations to reset the bus it's on -- e.g., not wanting that to occur in jail. /dev/kmem is particularly funky, since it inherently bypasses most aspects of the TCB -- even read access is quite dangerous, although not quite as overtly dangerous as write access. I'm not sure I see a strong argument for introducing further privileges here, as it's hard to imagine a situation where you'd delegate kmem a
ccess and not mean to give out vast amounts of privilege. The main argument for doing so would be that we wanted to pull securelevel checks inside of priv(9), sucking awareness of securelevels out of most consumer subsystems.

More generally, there are many implied sources of privilege in the system that don't go through priv(9) -- for example, uid 0 is not just privileged because it is granted rights by priv(9), but also because it owns lots of key files in the file system and can read/write them (e.g., the password file). You can similarly argue that although aspects of the TCB are protected by priv(9), others are protected by DAC (or optionally MAC). This is simply an "unclean" bit of the UNIX design, and something we end up accepting because the notion of TCB protection generally does involve some lack of cleanliness, since it has to do with protecting low-level parts of the system that aren't easily abstracted away (e.g., device drivers).

Robert

[Jamie Gritton]

On 06/17/13 15:02, Robert N. M. Watson wrote:
>
> On 16 Jun 2013, at 23:48, Kirk McKusick wrote:
>
>>> I suppose it's safe to say further comment isn't forthcoming. So with
>>> one vote for and one against (or at least questioning), I'll humbly
>>> leave it up to myself to be the tie-breaker :-).
>>>
>>> Here's a proposed patch. I separate kmem access into read and write, as
>>> I saw other similar splits in the priv list. Perhaps that's overkill,
>>> and I can use a single PRIV_KMEM instead of PRIV_KMEM_READ and
>>> PRIV_KMEM_WRITE.
>>>
>>> Perhaps this is an overreach, because PRIV_KMEM_READ is used where the
>>> default isn't root privilege: the file permission and expected usage are
>>> group kmem gets to read /dev/[k]mem. I'm not about to go hard-coding a
>>> gid into the kernel, so it seems the proper thing to do (not included in
>>> the patch) would be to allow PRIV_KMEM_READ by default. I thought there
>>> might already be such cases where the default is to allow, but no: this
>>> would be the first default-allow permission. So perhaps the best answer
>>> is not worry about that one, and only add PRIV_KMEM_WRITE (leaving reads
>>> controlled by file permission alone as they are now).
>>
>> With the change from the error noted by Kostik, I concur with your
>> proposed change.
>
> On the whole, the kernel privilege system is fairly straight-forward; the one real exception its its interaction with device-driver specific interfaces that seem privilege-like. On the whole, I take the view that we should define privileges in device drivers where either (a) there's a securelevel interaction or (b) it's likely we'll want to delegate access to the device node (perhaps in jail, or via chmod/chown) but there are special operations we still want to require privilege. For example, you could imagine wanting read/write access to a disk to be delegated, but not control operations to reset the bus it's on -- e.g., not wanting that to occur in jail. /dev/kmem is particularly funky, since it inherently bypasses most aspects of the TCB -- even read access is quite dangerous, although not quite as overtly dangerous as write access. I'm not sure I see a strong argument for introducing further privileges here, as it's hard to imagine a situation where you'd delegate kmem

access and not mean to give out vast amounts of privilege. The main argument for doing so would be that we wanted to pull securelevel checks inside of priv(9), sucking awareness of securelevels out of most consumer subsystems.

>
> More generally, there are many implied sources of privilege in the system that don't go through priv(9) -- for example, uid 0 is not just privileged because it is granted rights by priv(9), but also because it owns lots of key files in the file system and can read/write them (e.g., the password file). You can similarly argue that although aspects of the TCB are protected by priv(9), others are protected by DAC (or optionally MAC). This is simply an "unclean" bit of the UNIX design, and something we end up accepting because the notion of TCB protection generally does involve some lack of cleanliness, since it has to do with protecting low-level parts of the system that aren't easily abstracted away (e.g., device drivers).
>
> Robert

Am I correct in reading this as an argument against? Interestingly, you
mention securelevel interaction, and the writing case for kmem has that.
But my proposed patch left that part alone, just adding a priv check
alongside it. I had considered rolling the securelevel into priv, but I
didn't see anything else there that checked securelevel, so I left it alone.

- Jamie