Re: [PATCH] remove need for root to run DRM
On Thu, 2005-08-04 at 00:44 +0100, Dave Airlie wrote: > > restricted to the first process that opens the DRM device. Of couse > > that process may not be an Xserver. > > > > Can people add notes about possible security problems with each of these? > > > > You've missed all the driver ioctls.. please make a list of current driver > ioctls that need root as well.. > > I'm not over-the-moon about this approach of changing the system to be > default allow anything and adding root checks, I'd rather it was default > root check and overrideable to allow non-root... I'm mixed. I'm fine with having the root checks be special cases in the few ioctls that need it, when I've seen a review of every one from which the root check is being removed. The two flags in the ioctl descriptions currently are pretty unreadable, 3 would be even more confusing. Oh, and also this is as long as those root checks in the shared code get wrapped in an appropriate macro/function (taking no arguments). -- Eric Anholt [EMAIL PROTECTED] http://people.freebsd.org/~anholt/ [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
Root IOCTLs used by my first test program.
--
Jon Smirl
[EMAIL PROTECTED]
diff --git a/linux-core/drm_drv.c b/linux-core/drm_drv.c
--- a/linux-core/drm_drv.c
+++ b/linux-core/drm_drv.c
@@ -65,36 +65,36 @@ drm_ioctl_desc_t drm_ioctls[] = {
[DRM_IOCTL_NR(DRM_IOCTL_GET_MAP)] = {drm_getmap, 0, 0, 0},
[DRM_IOCTL_NR(DRM_IOCTL_GET_CLIENT)] = {drm_getclient, 0, 0, 0},
[DRM_IOCTL_NR(DRM_IOCTL_GET_STATS)] = {drm_getstats, 0, 0, 0},
- [DRM_IOCTL_NR(DRM_IOCTL_SET_VERSION)] = {drm_setversion, 0, 1, 1},
+ [DRM_IOCTL_NR(DRM_IOCTL_SET_VERSION)] = {drm_setversion, 0, 1, 0},
[DRM_IOCTL_NR(DRM_IOCTL_SET_UNIQUE)] = {drm_setunique, 1, 1, 1},
[DRM_IOCTL_NR(DRM_IOCTL_BLOCK)] = {drm_noop, 1, 1, 1},
[DRM_IOCTL_NR(DRM_IOCTL_UNBLOCK)] = {drm_noop, 1, 1, 1},
[DRM_IOCTL_NR(DRM_IOCTL_AUTH_MAGIC)] = {drm_authmagic, 1, 1, 1},
- [DRM_IOCTL_NR(DRM_IOCTL_ADD_MAP)] = {drm_addmap_ioctl, 1, 1, 1},
+ [DRM_IOCTL_NR(DRM_IOCTL_ADD_MAP)] = {drm_addmap_ioctl, 1, 1, 0},
[DRM_IOCTL_NR(DRM_IOCTL_RM_MAP)] = {drm_rmmap_ioctl, 1, 0, 0},
[DRM_IOCTL_NR(DRM_IOCTL_SET_SAREA_CTX)] = {drm_setsareactx, 1, 1, 1},
[DRM_IOCTL_NR(DRM_IOCTL_GET_SAREA_CTX)] = {drm_getsareactx, 1, 0, 0},
- [DRM_IOCTL_NR(DRM_IOCTL_ADD_CTX)] = {drm_addctx, 1, 1, 1},
- [DRM_IOCTL_NR(DRM_IOCTL_RM_CTX)] = {drm_rmctx, 1, 1, 1},
+ [DRM_IOCTL_NR(DRM_IOCTL_ADD_CTX)] = {drm_addctx, 1, 1, 0},
+ [DRM_IOCTL_NR(DRM_IOCTL_RM_CTX)] = {drm_rmctx, 1, 1, 0},
[DRM_IOCTL_NR(DRM_IOCTL_MOD_CTX)] = {drm_modctx, 1, 1, 1},
[DRM_IOCTL_NR(DRM_IOCTL_GET_CTX)] = {drm_getctx, 1, 0, 0},
[DRM_IOCTL_NR(DRM_IOCTL_SWITCH_CTX)] = {drm_switchctx, 1, 1, 1},
[DRM_IOCTL_NR(DRM_IOCTL_NEW_CTX)] = {drm_newctx, 1, 1, 1},
[DRM_IOCTL_NR(DRM_IOCTL_RES_CTX)] = {drm_resctx, 1, 0, 0},
- [DRM_IOCTL_NR(DRM_IOCTL_ADD_DRAW)] = {drm_adddraw, 1, 1, 1},
- [DRM_IOCTL_NR(DRM_IOCTL_RM_DRAW)] = {drm_rmdraw, 1, 1, 1},
+ [DRM_IOCTL_NR(DRM_IOCTL_ADD_DRAW)] = {drm_adddraw, 1, 1, 0},
+ [DRM_IOCTL_NR(DRM_IOCTL_RM_DRAW)] = {drm_rmdraw, 1, 1, 0},
[DRM_IOCTL_NR(DRM_IOCTL_LOCK)] = {drm_lock, 1, 0, 0},
[DRM_IOCTL_NR(DRM_IOCTL_UNLOCK)] = {drm_unlock, 1, 0, 0},
[DRM_IOCTL_NR(DRM_IOCTL_FINISH)] = {drm_noop, 1, 0, 0},
- [DRM_IOCTL_NR(DRM_IOCTL_ADD_BUFS)] = {drm_addbufs, 1, 1, 1},
+ [DRM_IOCTL_NR(DRM_IOCTL_ADD_BUFS)] = {drm_addbufs, 1, 1, 0},
[DRM_IOCTL_NR(DRM_IOCTL_MARK_BUFS)] = {drm_markbufs, 1, 1, 1},
[DRM_IOCTL_NR(DRM_IOCTL_INFO_BUFS)] = {drm_infobufs, 1, 0, 0},
[DRM_IOCTL_NR(DRM_IOCTL_MAP_BUFS)] = {drm_mapbufs, 1, 0, 0},
@@ -102,17 +102,17 @@ drm_ioctl_desc_t drm_ioctls[] = {
/* The DRM_IOCTL_DMA ioctl should be defined by the driver. */
[DRM_IOCTL_NR(DRM_IOCTL_DMA)] = {NULL, 1, 0, 0},
- [DRM_IOCTL_NR(DRM_IOCTL_CONTROL)] = {drm_control, 1, 1, 1},
+ [DRM_IOCTL_NR(DRM_IOCTL_CONTROL)] = {drm_control, 1, 1, 0},
#if __OS_HAS_AGP
- [DRM_IOCTL_NR(DRM_IOCTL_AGP_ACQUIRE)] = {drm_agp_acquire_ioctl, 1, 1,
1},
- [DRM_IOCTL_NR(DRM_IOCTL_AGP_RELEASE)] = {drm_agp_release_ioctl, 1, 1,
1},
- [DRM_IOCTL_NR(DRM_IOCTL_AGP_ENABLE)] = {drm_agp_enable_ioctl, 1, 1, 1},
+ [DRM_IOCTL_NR(DRM_IOCTL_AGP_ACQUIRE)] = {drm_agp_acquire_ioctl, 1, 1,
0},
+ [DRM_IOCTL_NR(DRM_IOCTL_AGP_RELEASE)] = {drm_agp_release_ioctl, 1, 1,
0},
+ [DRM_IOCTL_NR(DRM_IOCTL_AGP_ENABLE)] = {drm_agp_enable_ioctl, 1, 1, 0},
[DRM_IOCTL_NR(DRM_IOCTL_AGP_INFO)] = {drm_agp_info_ioctl, 1, 0, 0},
- [DRM_IOCTL_NR(DRM_IOCTL_AGP_ALLOC)] = {drm_agp_alloc, 1, 1, 1},
- [DRM_IOCTL_NR(DRM_IOCTL_AGP_FREE)] = {drm_agp_free, 1, 1, 1},
- [DRM_IOCTL_NR(DRM_IOCTL_AGP_BIND)] = {drm_agp_bind, 1, 1, 1},
- [DRM_IOCTL_NR(DRM_IOCTL_AGP_UNBIND)] = {drm_agp_unbind, 1, 1, 1},
+ [DRM_IOCTL_NR(DRM_IOCTL_AGP_ALLOC)] = {drm_agp_alloc, 1, 1, 0},
+ [DRM_IOCTL_NR(DRM_IOCTL_AGP_FREE)] = {drm_agp_free, 1, 1, 0},
+ [DRM_IOCTL_NR(DRM_IOCTL_AGP_BIND)] = {drm_agp_bind, 1, 1, 0},
+ [DRM_IOCTL_NR(DRM_IOCTL_AGP_UNBIND)] = {drm_agp_unbind, 1, 1, 0},
#endif
[DRM_IOCTL_NR(DRM_IOCTL_SG_ALLOC)] = {drm_sg_alloc, 1, 1, 1},
diff --git a/shared-core/radeon_state.c b/shared-core/radeon_state.c
--- a/shared-core/radeon_state.c
+++ b/shared-core/radeon_state.c
@@ -3056,9 +3056,9 @@ void radeon_driver_free_filp_priv(drm_de
}
drm_ioctl_desc_t radeon_ioctls[] = {
- [DRM_IOCTL_NR(DRM_RADEON_CP_INIT)] = {radeon_cp_init, 1, 1, 1},
- [DRM_IOCTL_NR(DRM_RADEON_CP_START)] = {radeon_cp_start, 1, 1, 1},
- [DRM_IOCTL_NR(DRM_RADEON_CP_STOP)] = {radeon_cp_stop, 1, 1, 1},
+ [DRM_IOCTL_NR(DRM_RADEON_CP_INIT)] = {radeon_cp_init, 1, 1, 0},
+ [DRM_IOCTL_NR(DRM_RADEON_CP_START)] = {radeon_cp_start, 1, 1, 0},
+ [DRM_IOCTL_NR(DRM_RADEON_CP_STOP)] = {radeon_cp_stop, 1, 1, 0},
[DRM_IOCTL_NR(DRM_RADEON_CP_RESET)] = {radeon_cp_reset, 1, 1, 1},
Re: [PATCH] remove need for root to run DRM
On 8/3/05, Ian Romanick <[EMAIL PROTECTED]> wrote: > -BEGIN PGP SIGNED MESSAGE- > Hash: SHA1 > > Jon Smirl wrote: > > On 8/3/05, Dave Airlie <[EMAIL PROTECTED]> wrote: > > > >>I'm not over-the-moon about this approach of changing the system to be > >>default allow anything and adding root checks, I'd rather it was default > >>root check and overrideable to allow non-root... > > > > I started off that way but then I figured out that very few ioctl need > > to require root. That would require adding about 70 root checks and > > then turning around and eliminating most of them immediately since > > mesa uses almost all of the ioctls (indirect is the only exception I > > know of) We can get the same effect just by inspecting the list of > > ioctls. > > The difference being that you can incrementally remove root-checks > without compromising the system. The same cannot be said for > incrementally adding them. Alright I will add them, but I am not convinced we will learn anything about where the vulnerabilities are. But it will have the effect of leaving the non-radeon drivers as root only until they are converted for EGL use. > -BEGIN PGP SIGNATURE- > Version: GnuPG v1.2.6 (GNU/Linux) > Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org > > iD8DBQFC8WfyX1gOwKyEAw8RAsl8AJ9BpLonnpNTdETFS/C5zmHAxr/9gwCcD0wa > mab3pazwkd13LCmYcDgFAUM= > =C9NI > -END PGP SIGNATURE- > -- Jon Smirl [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Jon Smirl wrote: > On 8/3/05, Dave Airlie <[EMAIL PROTECTED]> wrote: > >>I'm not over-the-moon about this approach of changing the system to be >>default allow anything and adding root checks, I'd rather it was default >>root check and overrideable to allow non-root... > > I started off that way but then I figured out that very few ioctl need > to require root. That would require adding about 70 root checks and > then turning around and eliminating most of them immediately since > mesa uses almost all of the ioctls (indirect is the only exception I > know of) We can get the same effect just by inspecting the list of > ioctls. The difference being that you can incrementally remove root-checks without compromising the system. The same cannot be said for incrementally adding them. -BEGIN PGP SIGNATURE- Version: GnuPG v1.2.6 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iD8DBQFC8WfyX1gOwKyEAw8RAsl8AJ9BpLonnpNTdETFS/C5zmHAxr/9gwCcD0wa mab3pazwkd13LCmYcDgFAUM= =C9NI -END PGP SIGNATURE- --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On 8/3/05, Dave Airlie <[EMAIL PROTECTED]> wrote: > > restricted to the first process that opens the DRM device. Of couse > > that process may not be an Xserver. > > > > Can people add notes about possible security problems with each of these? > > > > You've missed all the driver ioctls.. please make a list of current driver > ioctls that need root as well.. I was saving them until we went through the base ones first. > > I'm not over-the-moon about this approach of changing the system to be > default allow anything and adding root checks, I'd rather it was default > root check and overrideable to allow non-root... I started off that way but then I figured out that very few ioctl need to require root. That would require adding about 70 root checks and then turning around and eliminating most of them immediately since mesa uses almost all of the ioctls (indirect is the only exception I know of) We can get the same effect just by inspecting the list of ioctls. > > Dave. > > -- > David Airlie, Software Engineer > http://www.skynet.ie/~airlied / airlied at skynet.ie > Linux kernel - DRI, VAX / pam_smb / ILUG > > > > --- > SF.Net email is Sponsored by the Better Software Conference & EXPO > September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices > Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA > Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf > -- > ___ > Dri-devel mailing list > Dri-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/dri-devel > -- Jon Smirl [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
> restricted to the first process that opens the DRM device. Of couse > that process may not be an Xserver. > > Can people add notes about possible security problems with each of these? > You've missed all the driver ioctls.. please make a list of current driver ioctls that need root as well.. I'm not over-the-moon about this approach of changing the system to be default allow anything and adding root checks, I'd rather it was default root check and overrideable to allow non-root... Dave. -- David Airlie, Software Engineer http://www.skynet.ie/~airlied / airlied at skynet.ie Linux kernel - DRI, VAX / pam_smb / ILUG --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On Wed, 2005-08-03 at 17:10 -0400, Jon Smirl wrote: > On 8/3/05, Michel Dänzer <[EMAIL PROTECTED]> wrote: > > On Wed, 2005-08-03 at 16:18 -0400, Jon Smirl wrote: > > > On 8/3/05, Michel Dänzer <[EMAIL PROTECTED]> wrote: > > > > > > > > > > They aren't used in the mesa tree. > > > > > > > > So why did you change their requiring root? > > > > > > The version of Xegl I am making does not run as root. [...] > > > > I know. You missed my question: Why do you change the behaviour of code > > that doesn't affect what you're trying to achieve? > > The original code did not separate the concept of auth and root, they > were implemented as the same bit. I had to separate the concepts. I > kept all of the code implementing auth unchanged. > > There was a single check looking for root across all IOCTLs. I had to > remove that check. Now we have have to identify the IOCTLs that > really require root and add the check specifically to them. So far > there are only two: addmap and indirect. > > I could have made three bits: auth_needed, root_only, master. But > that was a lot of deltas to implement a root_only bit which is only > needed for indirect. Instead it is easier to just add a capability > root check in the ioctl. In your previous patch you removed the root check entirely, even though that lead to vulnerabilities. I pointed out two cases, but I didn't review all the ioctls. Before a patch based on this goes in, I would like a review of every previously root-requiring ioctl to explain why it's okay to not require root on it now. Alternatively, you could do what Michel suggested: make only the changes that are required for your nonroot case, so that the security implications are (relatively) obvious. -- Eric Anholt [EMAIL PROTECTED] http://people.freebsd.org/~anholt/ [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
Here are the main IOCTLs that used to require root. Now they are restricted to the first process that opens the DRM device. Of couse that process may not be an Xserver. Can people add notes about possible security problems with each of these? DRM_IOCTL_IRQ_BUSID DRM_IOCTL_SET_VERSION DRM_IOCTL_SET_UNIQUE DRM_IOCTL_BLOCK DRM_IOCTL_UNBLOCK DRM_IOCTL_AUTH_MAGIC DRM_IOCTL_ADD_MAP - still does in most cases DRM_IOCTL_SET_SAREA_CTX DRM_IOCTL_ADD_CTX DRM_IOCTL_RM_CTX DRM_IOCTL_MOD_CTX DRM_IOCTL_SWITCH_CTX DRM_IOCTL_NEW_CTX DRM_IOCTL_ADD_DRAW DRM_IOCTL_RM_DRAW DRM_IOCTL_ADD_BUFS DRM_IOCTL_MARK_BUFS DRM_IOCTL_CONTROL DRM_IOCTL_AGP_ACQUIRE DRM_IOCTL_AGP_RELEASE DRM_IOCTL_AGP_ENABLE DRM_IOCTL_AGP_ALLOC DRM_IOCTL_AGP_FREE DRM_IOCTL_AGP_BIND DRM_IOCTL_AGP_UNBIND DRM_IOCTL_SG_ALLOC DRM_IOCTL_SG_FREE -- Jon Smirl [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On 8/3/05, Eric Anholt <[EMAIL PROTECTED]> wrote: > On Wed, 2005-08-03 at 17:10 -0400, Jon Smirl wrote: > > On 8/3/05, Michel Dänzer <[EMAIL PROTECTED]> wrote: > > > On Wed, 2005-08-03 at 16:18 -0400, Jon Smirl wrote: > > > > On 8/3/05, Michel Dänzer <[EMAIL PROTECTED]> wrote: > > > > > > > > > > > > They aren't used in the mesa tree. > > > > > > > > > > So why did you change their requiring root? > > > > > > > > The version of Xegl I am making does not run as root. [...] > > > > > > I know. You missed my question: Why do you change the behaviour of code > > > that doesn't affect what you're trying to achieve? > > > > The original code did not separate the concept of auth and root, they > > were implemented as the same bit. I had to separate the concepts. I > > kept all of the code implementing auth unchanged. > > > > There was a single check looking for root across all IOCTLs. I had to > > remove that check. Now we have have to identify the IOCTLs that > > really require root and add the check specifically to them. So far > > there are only two: addmap and indirect. > > > > I could have made three bits: auth_needed, root_only, master. But > > that was a lot of deltas to implement a root_only bit which is only > > needed for indirect. Instead it is easier to just add a capability > > root check in the ioctl. > > In your previous patch you removed the root check entirely, even though > that lead to vulnerabilities. I pointed out two cases, but I didn't > review all the ioctls. Before a patch based on this goes in, I would > like a review of every previously root-requiring ioctl to explain why > it's okay to not require root on it now. I have been asking on this list for a month now for everyone to help locate where there are vulnerabilities with dropping root priv. Multiple people have told me that AddMap was the only problem. You just pointed out another one with radeon/r128 indirect. > Alternatively, you could do what Michel suggested: make only the changes > that are required for your nonroot case, so that the security > implications are (relatively) obvious. Mesa hits every main DRM entry point. I believe the problem is now with the drivers. Are there other X only driver entry points? It is more reliable if you can just tell me what are the likely problem areas than it is for me to go grepping around xorg trying to figure out what it uses. Any ioctl that is X only can be set to require root priv. > > -- > Eric Anholt [EMAIL PROTECTED] > http://people.freebsd.org/~anholt/ [EMAIL PROTECTED] > -- Jon Smirl [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
New version that protects r128 and radeon IOCTLs with root capablity
check. Please review this patch. We need everyone's eyes to make sure
there are no accidental security holes.
--
Jon Smirl
[EMAIL PROTECTED]
diff --git a/linux-core/drmP.h b/linux-core/drmP.h
--- a/linux-core/drmP.h
+++ b/linux-core/drmP.h
@@ -280,7 +280,7 @@ typedef int drm_ioctl_compat_t(struct fi
typedef struct drm_ioctl_desc {
drm_ioctl_t *func;
int auth_needed;
- int root_only;
+ int master;
} drm_ioctl_desc_t;
typedef struct drm_devstate {
@@ -375,6 +375,7 @@ typedef struct drm_buf_entry {
/** File private data */
typedef struct drm_file {
int authenticated;
+ int master;
int minor;
pid_t pid;
uid_t uid;
diff --git a/linux-core/drm_bufs.c b/linux-core/drm_bufs.c
--- a/linux-core/drm_bufs.c
+++ b/linux-core/drm_bufs.c
@@ -56,7 +56,8 @@ static drm_local_map_t *drm_find_matchin
list_for_each(list, &dev->maplist->head) {
drm_map_list_t *entry = list_entry(list, drm_map_list_t, head);
if (entry->map && map->type == entry->map->type &&
- entry->map->offset == map->offset) {
+ ((entry->map->offset == map->offset) ||
+(map->type == _DRM_SHM))) {
return entry->map;
}
}
@@ -163,6 +164,19 @@ int drm_addmap(drm_device_t * dev, unsig
map->handle = drm_ioremap(map->offset, map->size, dev);
break;
case _DRM_SHM:
+ found_map = drm_find_matching_map(dev, map);
+ if (found_map != NULL) {
+ if (found_map->size != map->size) {
+ DRM_DEBUG("Matching maps of type %d with "
+ "mismatched sizes, (%ld vs %ld)\n",
+ map->type, map->size, found_map->size);
+ found_map->size = map->size;
+ }
+
+ drm_free(map, sizeof(*map), DRM_MEM_MAPS);
+ *map_ptr = found_map;
+ return 0;
+ }
map->handle = vmalloc_32(map->size);
DRM_DEBUG("%lu %d %p\n",
map->size, drm_order(map->size), map->handle);
@@ -181,15 +195,34 @@ int drm_addmap(drm_device_t * dev, unsig
dev->sigdata.lock = dev->lock.hw_lock = map->handle;
/* Pointer to lock */
}
break;
- case _DRM_AGP:
- if (drm_core_has_AGP(dev)) {
+ case _DRM_AGP: {
+ drm_agp_mem_t *entry;
+ int valid = 0;
+
+ if (!drm_core_has_AGP(dev)) {
+ drm_free(map, sizeof(*map), DRM_MEM_MAPS);
+ return -EINVAL;
+ }
#ifdef __alpha__
- map->offset += dev->hose->mem_space->start;
+ map->offset += dev->hose->mem_space->start;
#endif
- map->offset += dev->agp->base;
- map->mtrr = dev->agp->agp_mtrr; /* for getmap */
+ map->offset += dev->agp->base;
+ map->mtrr = dev->agp->agp_mtrr; /* for getmap */
+
+ for (entry = dev->agp->memory; entry; entry = entry->next) {
+ if ((map->offset >= entry->bound) &&
+ (map->offset + map->offset <= entry->bound +
entry->pages *
PAGE_SIZE)) {
+ valid = 1;
+ break;
+ }
+ }
+ if (!valid) {
+ drm_free(map, sizeof(*map), DRM_MEM_MAPS);
+ return -EPERM;
}
+ DRM_DEBUG("AGP offset = 0x%08lx, size = 0x%08lx\n",
map->offset, map->size);
break;
+ }
case _DRM_SCATTER_GATHER:
if (!dev->sg) {
drm_free(map, sizeof(*map), DRM_MEM_MAPS);
@@ -258,6 +291,9 @@ int drm_addmap_ioctl(struct inode *inode
return -EFAULT;
}
+ if (!(capable(CAP_SYS_ADMIN) || map.type == _DRM_AGP))
+ return -EPERM;
+
err = drm_addmap( dev, map.offset, map.size, map.type, map.flags,
& map_ptr );
diff --git a/linux-core/drm_drv.c b/linux-core/drm_drv.c
--- a/linux-core/drm_drv.c
+++ b/linux-core/drm_drv.c
@@ -545,13 +545,14 @@ int drm_ioctl(struct inode *inode, struc
if (!func) {
DRM_DEBUG("no function\n");
retcode = -EINVAL;
- } else if ((ioctl->root_only && !capable(CAP_SYS_ADMIN)) ||
- (ioctl->auth_needed && !priv->authenticated)) {
+ } else if (((ioctl->master && !priv->master) ||
+ (ioctl->auth_needed && !priv->authenticated)) &&
+ (!capable(CAP_SYS_ADMIN))) {
r
Re: [PATCH] remove need for root to run DRM
On 8/3/05, Michel Dänzer <[EMAIL PROTECTED]> wrote: > On Wed, 2005-08-03 at 15:02 -0400, Jon Smirl wrote: > > On 8/3/05, Eric Anholt <[EMAIL PROTECTED]> wrote: > > > On Wed, 2005-08-03 at 14:39 -0400, Jon Smirl wrote: > > > > > ioctls where removing the root check introduces privelege escalation > > > > > for > > > > > users with read access to the DRM device (at least): > > > > > - DRM_R128_INDIRECT > > > > > - DRM_RADEON_INDIRECT > > > > > > > > How do we secure these? > > > > > > By requiring root. But I didn't review all the ioctls, so these might > > > not be all of the root-requiring ioctls that continue to need it. > > > > I thought we built a command verifier to check things like this. > > These ioctls are designed for privileged clients like the current DDX > drivers and thus unchecked. Ok, that's not inconsistent with what I am trying to do. I can just add a root capability check on those two IOCTLs. From IRC see I see that they are only used by the Xserver internally. Mesa doesn't need them. -- Jon Smirl [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On 8/3/05, Michel Dänzer <[EMAIL PROTECTED]> wrote: > On Wed, 2005-08-03 at 16:18 -0400, Jon Smirl wrote: > > On 8/3/05, Michel Dänzer <[EMAIL PROTECTED]> wrote: > > > > > > > > They aren't used in the mesa tree. > > > > > > So why did you change their requiring root? > > > > The version of Xegl I am making does not run as root. [...] > > I know. You missed my question: Why do you change the behaviour of code > that doesn't affect what you're trying to achieve? The original code did not separate the concept of auth and root, they were implemented as the same bit. I had to separate the concepts. I kept all of the code implementing auth unchanged. There was a single check looking for root across all IOCTLs. I had to remove that check. Now we have have to identify the IOCTLs that really require root and add the check specifically to them. So far there are only two: addmap and indirect. I could have made three bits: auth_needed, root_only, master. But that was a lot of deltas to implement a root_only bit which is only needed for indirect. Instead it is easier to just add a capability root check in the ioctl. You can't uses a root only bit on addmap since the root requirement is a function of the parameters passed in. -- Jon Smirl [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On Wed, 2005-08-03 at 16:18 -0400, Jon Smirl wrote: > On 8/3/05, Michel Dänzer <[EMAIL PROTECTED]> wrote: > > > > > > They aren't used in the mesa tree. > > > > So why did you change their requiring root? > > The version of Xegl I am making does not run as root. [...] I know. You missed my question: Why do you change the behaviour of code that doesn't affect what you're trying to achieve? -- Earthling Michel Dänzer | Debian (powerpc), X and DRI developer Libre software enthusiast| http://svcs.affero.net/rm.php?r=daenzer --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On 8/3/05, Michel Dänzer <[EMAIL PROTECTED]> wrote: > On Wed, 2005-08-03 at 15:14 -0400, Jon Smirl wrote: > > On 8/3/05, Eric Anholt <[EMAIL PROTECTED]> wrote: > > > > > > These are the indirect ioctls, which allow the X Server to submit a > > > buffer of any commands it wants. You could probably build a (or extend > > > the current) verifier for the all the things the X Server has done > > > through that ioctl, but that hasn't been done. > > > > So there is probably a general security hole here if I can convice the > > Xserver to use the buffer addresses I want. > > That would require a security hole in the X server. The attacker is root > already in that case. > > > Who uses these? > > The current DDX drivers. > > > They aren't used in the mesa tree. > > So why did you change their requiring root? The version of Xegl I am making does not run as root. It handles multiuser by letting each user run their own instance of Xegl as a normal app. To make this work I have to modify DRM to not need root priv to run. I removed the general root priv check that covered all ioctls. Now we need to review and add it back individually on the ones that need it. For example AddMap was modified to allow mesa to work without root but some of the things X does still need root. The indirect ioctls are not used by mesa. In a little while I'll put together a new patch that adds the root requirement back on those ioctls. -- Jon Smirl [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On Wed, 2005-08-03 at 15:14 -0400, Jon Smirl wrote: > On 8/3/05, Eric Anholt <[EMAIL PROTECTED]> wrote: > > > > These are the indirect ioctls, which allow the X Server to submit a > > buffer of any commands it wants. You could probably build a (or extend > > the current) verifier for the all the things the X Server has done > > through that ioctl, but that hasn't been done. > > So there is probably a general security hole here if I can convice the > Xserver to use the buffer addresses I want. That would require a security hole in the X server. The attacker is root already in that case. > Who uses these? The current DDX drivers. > They aren't used in the mesa tree. So why did you change their requiring root? -- Earthling Michel Dänzer | Debian (powerpc), X and DRI developer Libre software enthusiast| http://svcs.affero.net/rm.php?r=daenzer --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On 8/3/05, Eric Anholt <[EMAIL PROTECTED]> wrote: > On Wed, 2005-08-03 at 15:02 -0400, Jon Smirl wrote: > > On 8/3/05, Eric Anholt <[EMAIL PROTECTED]> wrote: > > > On Wed, 2005-08-03 at 14:39 -0400, Jon Smirl wrote: > > > > > ioctls where removing the root check introduces privelege escalation > > > > > for > > > > > users with read access to the DRM device (at least): > > > > > - DRM_R128_INDIRECT > > > > > - DRM_RADEON_INDIRECT > > > > > > > > How do we secure these? > > > > > > By requiring root. But I didn't review all the ioctls, so these might > > > not be all of the root-requiring ioctls that continue to need it. > > > > I thought we built a command verifier to check things like this. Does > > DRM need to copy, verify, then > > These are the indirect ioctls, which allow the X Server to submit a > buffer of any commands it wants. You could probably build a (or extend > the current) verifier for the all the things the X Server has done > through that ioctl, but that hasn't been done. So there is probably a general security hole here if I can convice the Xserver to use the buffer addresses I want. Who uses these? They aren't used in the mesa tree. > > -- > Eric Anholt [EMAIL PROTECTED] > http://people.freebsd.org/~anholt/ [EMAIL PROTECTED] > -- Jon Smirl [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On Wed, 2005-08-03 at 15:02 -0400, Jon Smirl wrote: > On 8/3/05, Eric Anholt <[EMAIL PROTECTED]> wrote: > > On Wed, 2005-08-03 at 14:39 -0400, Jon Smirl wrote: > > > > ioctls where removing the root check introduces privelege escalation for > > > > users with read access to the DRM device (at least): > > > > - DRM_R128_INDIRECT > > > > - DRM_RADEON_INDIRECT > > > > > > How do we secure these? > > > > By requiring root. But I didn't review all the ioctls, so these might > > not be all of the root-requiring ioctls that continue to need it. > > I thought we built a command verifier to check things like this. These ioctls are designed for privileged clients like the current DDX drivers and thus unchecked. -- Earthling Michel Dänzer | Debian (powerpc), X and DRI developer Libre software enthusiast| http://svcs.affero.net/rm.php?r=daenzer --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On Wed, 2005-08-03 at 15:02 -0400, Jon Smirl wrote: > On 8/3/05, Eric Anholt <[EMAIL PROTECTED]> wrote: > > On Wed, 2005-08-03 at 14:39 -0400, Jon Smirl wrote: > > > > ioctls where removing the root check introduces privelege escalation for > > > > users with read access to the DRM device (at least): > > > > - DRM_R128_INDIRECT > > > > - DRM_RADEON_INDIRECT > > > > > > How do we secure these? > > > > By requiring root. But I didn't review all the ioctls, so these might > > not be all of the root-requiring ioctls that continue to need it. > > I thought we built a command verifier to check things like this. Does > DRM need to copy, verify, then These are the indirect ioctls, which allow the X Server to submit a buffer of any commands it wants. You could probably build a (or extend the current) verifier for the all the things the X Server has done through that ioctl, but that hasn't been done. -- Eric Anholt [EMAIL PROTECTED] http://people.freebsd.org/~anholt/ [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On 8/3/05, Eric Anholt <[EMAIL PROTECTED]> wrote: > On Wed, 2005-08-03 at 14:39 -0400, Jon Smirl wrote: > > > ioctls where removing the root check introduces privelege escalation for > > > users with read access to the DRM device (at least): > > > - DRM_R128_INDIRECT > > > - DRM_RADEON_INDIRECT > > > > How do we secure these? > > By requiring root. But I didn't review all the ioctls, so these might > not be all of the root-requiring ioctls that continue to need it. I thought we built a command verifier to check things like this. Does DRM need to copy, verify, then execute? > > -- > Eric Anholt [EMAIL PROTECTED] > http://people.freebsd.org/~anholt/ [EMAIL PROTECTED] > -- Jon Smirl [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On Wed, 2005-08-03 at 14:39 -0400, Jon Smirl wrote: > > ioctls where removing the root check introduces privelege escalation for > > users with read access to the DRM device (at least): > > - DRM_R128_INDIRECT > > - DRM_RADEON_INDIRECT > > How do we secure these? By requiring root. But I didn't review all the ioctls, so these might not be all of the root-requiring ioctls that continue to need it. -- Eric Anholt [EMAIL PROTECTED] http://people.freebsd.org/~anholt/ [EMAIL PROTECTED] --- SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
I forgot to add this in the first patch, you will GPF without it. The
attachment includes it.
@@ -72,7 +80,7 @@ static int drm_setup(drm_device_t * dev)
INIT_LIST_HEAD(&dev->ctxlist->head);
dev->vmalist = NULL;
- dev->sigdata.lock = dev->lock.hw_lock = NULL;
+ dev->sigdata.lock = NULL;
init_waitqueue_head(&dev->lock.lock_queue);
dev->queue_count = 0;
dev->queue_reserved = 0;
--
Jon Smirl
[EMAIL PROTECTED]
diff --git a/linux-core/drmP.h b/linux-core/drmP.h
--- a/linux-core/drmP.h
+++ b/linux-core/drmP.h
@@ -280,7 +280,7 @@ typedef int drm_ioctl_compat_t(struct fi
typedef struct drm_ioctl_desc {
drm_ioctl_t *func;
int auth_needed;
- int root_only;
+ int master;
} drm_ioctl_desc_t;
typedef struct drm_devstate {
@@ -375,6 +375,7 @@ typedef struct drm_buf_entry {
/** File private data */
typedef struct drm_file {
int authenticated;
+ int master;
int minor;
pid_t pid;
uid_t uid;
diff --git a/linux-core/drm_bufs.c b/linux-core/drm_bufs.c
--- a/linux-core/drm_bufs.c
+++ b/linux-core/drm_bufs.c
@@ -56,7 +56,8 @@ static drm_local_map_t *drm_find_matchin
list_for_each(list, &dev->maplist->head) {
drm_map_list_t *entry = list_entry(list, drm_map_list_t, head);
if (entry->map && map->type == entry->map->type &&
- entry->map->offset == map->offset) {
+ ((entry->map->offset == map->offset) ||
+ (map->type == _DRM_SHM))) {
return entry->map;
}
}
@@ -163,6 +164,19 @@ int drm_addmap(drm_device_t * dev, unsig
map->handle = drm_ioremap(map->offset, map->size, dev);
break;
case _DRM_SHM:
+ found_map = drm_find_matching_map(dev, map);
+ if (found_map != NULL) {
+ if (found_map->size != map->size) {
+DRM_DEBUG("Matching maps of type %d with "
+ "mismatched sizes, (%ld vs %ld)\n",
+map->type, map->size, found_map->size);
+found_map->size = map->size;
+ }
+
+ drm_free(map, sizeof(*map), DRM_MEM_MAPS);
+ *map_ptr = found_map;
+ return 0;
+ }
map->handle = vmalloc_32(map->size);
DRM_DEBUG("%lu %d %p\n",
map->size, drm_order(map->size), map->handle);
@@ -181,15 +195,34 @@ int drm_addmap(drm_device_t * dev, unsig
dev->sigdata.lock = dev->lock.hw_lock = map->handle; /* Pointer to lock */
}
break;
- case _DRM_AGP:
- if (drm_core_has_AGP(dev)) {
+ case _DRM_AGP: {
+ drm_agp_mem_t *entry;
+ int valid = 0;
+
+ if (!drm_core_has_AGP(dev)) {
+ drm_free(map, sizeof(*map), DRM_MEM_MAPS);
+ return -EINVAL;
+ }
#ifdef __alpha__
- map->offset += dev->hose->mem_space->start;
+ map->offset += dev->hose->mem_space->start;
#endif
- map->offset += dev->agp->base;
- map->mtrr = dev->agp->agp_mtrr; /* for getmap */
+ map->offset += dev->agp->base;
+ map->mtrr = dev->agp->agp_mtrr; /* for getmap */
+
+ for (entry = dev->agp->memory; entry; entry = entry->next) {
+ if ((map->offset >= entry->bound) &&
+ (map->offset + map->offset <= entry->bound + entry->pages * PAGE_SIZE)) {
+valid = 1;
+break;
+ }
+ }
+ if (!valid) {
+ drm_free(map, sizeof(*map), DRM_MEM_MAPS);
+ return -EPERM;
}
+ DRM_DEBUG("AGP offset = 0x%08lx, size = 0x%08lx\n", map->offset, map->size);
break;
+ }
case _DRM_SCATTER_GATHER:
if (!dev->sg) {
drm_free(map, sizeof(*map), DRM_MEM_MAPS);
@@ -258,6 +291,9 @@ int drm_addmap_ioctl(struct inode *inode
return -EFAULT;
}
+ if (!(capable(CAP_SYS_ADMIN) || map.type == _DRM_AGP))
+ return -EPERM;
+
err = drm_addmap( dev, map.offset, map.size, map.type, map.flags,
& map_ptr );
diff --git a/linux-core/drm_drv.c b/linux-core/drm_drv.c
--- a/linux-core/drm_drv.c
+++ b/linux-core/drm_drv.c
@@ -545,13 +545,14 @@ int drm_ioctl(struct inode *inode, struc
if (!func) {
DRM_DEBUG("no function\n");
retcode = -EINVAL;
- } else if ((ioctl->root_only && !capable(CAP_SYS_ADMIN)) ||
- (ioctl->auth_needed && !priv->authenticated)) {
+ } else if (((ioctl->master && !priv->master) ||
+ (ioctl->auth_needed && !priv->authenticated)) &&
+ (!capable(CAP_SYS_ADMIN))) {
retcode = -EACCES;
} else {
retcode = func(inode, filp, cmd, arg);
}
- err_i1:
+err_i1:
atomic_dec(&dev->ioctl_count);
if (retcode)
DRM_DEBUG("ret = %x\n", retcode);
diff --git a/linux-core/drm_fops.c b/linux-core/drm_fops.c
--- a/linux-core/drm_fops.c
+++ b/linux-core/drm_fops.c
@@ -34,18 +34,26 @@
* OTHER DEALINGS IN THE SOFTWARE.
*/
-#include "drmP.h"
#include
+#include "drmP.h"
+#include "drm_sarea.h"
+
static int drm_open_helper(struct inode *inode, struct file *filp, drm_device_t * dev);
static int drm_setup(drm_device_t * dev)
{
+ drm_local_map_t *map;
int i;
if (dev->driver->presetup)
dev->driver->presetup(dev);
+ /* prebuild the SAREA */
+ i = drm_addmap(dev, 0, SAREA_MAX, _DRM_SHM, _DRM_CONTAINS_LOCK, &map);
+ if (i != 0)
+ return i;
+
atomic_set(&dev->ioctl_count, 0);
atomic_set(&dev->vma_count, 0);
dev->buf_use = 0;
@@ -72,7 +80,7 @@ static int drm_setup(drm_device_t * dev
Re: [PATCH] remove need for root to run DRM
On 8/3/05, Eric Anholt <[EMAIL PROTECTED]> wrote:
> > @@ -163,6 +164,19 @@ int drm_addmap(drm_device_t * dev, unsig
> > map->handle = drm_ioremap(map->offset, map->size,
> > dev);
> > break;
> > case _DRM_SHM:
> > + found_map = drm_find_matching_map(dev, map);
> > + if (found_map != NULL) {
> > + if (found_map->size != map->size) {
> > + DRM_DEBUG("Matching maps of type %d with "
> > +"mismatched sizes, (%ld vs %ld)\n",
> > + map->type, map->size, found_map->size);
> > + found_map->size = map->size;
> > + }
> > +
> > + drm_free(map, sizeof(*map), DRM_MEM_MAPS);
> > + *map_ptr = found_map;
> > + return 0;
> > + }
>
> I'm uncomfortable with this "matching maps with mismatched sizes" code
> that now exists in 2 places, and would like to see what the reasoning is
> behind it.
Existing Xservers ask for maps that are not the correct size. This
lets them keep working.
>
> > diff --git a/shared-core/radeon_cp.c b/shared-core/radeon_cp.c
> > --- a/shared-core/radeon_cp.c
> > +++ b/shared-core/radeon_cp.c
> > @@ -1245,7 +1245,7 @@ static void radeon_set_pciegart(drm_rade
> > u32 tmp = RADEON_READ_PCIE(dev_priv, RADEON_PCIE_TX_GART_CNTL);
> > if (on) {
> >
> > - DRM_DEBUG("programming pcie %08X %08lX %08X\n",
> > dev_priv->gart_vm_start, dev_priv->bus_pci_gart,dev_priv->gart_size);
> > + DRM_DEBUG("programming pcie %08X %08X %08X\n",
> > dev_priv->gart_vm_start, dev_priv->bus_pci_gart,dev_priv->gart_size);
> > RADEON_WRITE_PCIE(RADEON_PCIE_TX_DISCARD_RD_ADDR_LO,
> > dev_priv->gart_vm_start);
> > RADEON_WRITE_PCIE(RADEON_PCIE_TX_GART_BASE,
> > dev_priv->bus_pci_gart);
> > RADEON_WRITE_PCIE(RADEON_PCIE_TX_GART_START_LO,
> > dev_priv->gart_vm_start);
>
> A dma_addr_t (dev_priv->bus_pci_gart) is a long on at least some
> systems. While we may know that it's 32 bits here, a cast will be
> needed to avoid warnings.
I was getting a warning in my build.
>
> ioctls where removing the root check introduces privelege escalation for
> users with read access to the DRM device (at least):
> - DRM_R128_INDIRECT
> - DRM_RADEON_INDIRECT
How do we secure these?
--
Jon Smirl
[EMAIL PROTECTED]
---
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] remove need for root to run DRM
On Wed, 2005-08-03 at 12:58 -0400, Jon Smirl wrote:
> This patch removes the need for root to run DRM. It is attached too in
> case gmail destroys the formatting.
>
> Major highlights:
> 1) removal of the general root check on the IOCTLs, added root check in AddMap
> 1) permanent SAREA map
I like that the SAREA setup was in the setup hook. Thanks.
> 2) user space can AddMap AGP maps, AGP maps are restricted to creation
> in AGP space.
> 3) Deprecation of some radeon variables now calculated by the driver
>
> I haven't converted PCI GART support. You still need to be root for it to
> work.
>
> Please look it over and tell me if I have created any security holes.
>
> --
> Jon Smirl
> [EMAIL PROTECTED]
> @@ -163,6 +164,19 @@ int drm_addmap(drm_device_t * dev, unsig
> map->handle = drm_ioremap(map->offset, map->size, dev);
> break;
> case _DRM_SHM:
> + found_map = drm_find_matching_map(dev, map);
> + if (found_map != NULL) {
> + if (found_map->size != map->size) {
> + DRM_DEBUG("Matching maps of type %d with "
> +"mismatched sizes, (%ld vs %ld)\n",
> + map->type, map->size, found_map->size);
> + found_map->size = map->size;
> + }
> +
> + drm_free(map, sizeof(*map), DRM_MEM_MAPS);
> + *map_ptr = found_map;
> + return 0;
> + }
I'm uncomfortable with this "matching maps with mismatched sizes" code
that now exists in 2 places, and would like to see what the reasoning is
behind it.
> diff --git a/shared-core/radeon_cp.c b/shared-core/radeon_cp.c
> --- a/shared-core/radeon_cp.c
> +++ b/shared-core/radeon_cp.c
> @@ -1245,7 +1245,7 @@ static void radeon_set_pciegart(drm_rade
> u32 tmp = RADEON_READ_PCIE(dev_priv, RADEON_PCIE_TX_GART_CNTL);
> if (on) {
>
> - DRM_DEBUG("programming pcie %08X %08lX %08X\n",
> dev_priv->gart_vm_start, dev_priv->bus_pci_gart,dev_priv->gart_size);
> + DRM_DEBUG("programming pcie %08X %08X %08X\n",
> dev_priv->gart_vm_start, dev_priv->bus_pci_gart,dev_priv->gart_size);
> RADEON_WRITE_PCIE(RADEON_PCIE_TX_DISCARD_RD_ADDR_LO,
> dev_priv->gart_vm_start);
> RADEON_WRITE_PCIE(RADEON_PCIE_TX_GART_BASE,
> dev_priv->bus_pci_gart);
> RADEON_WRITE_PCIE(RADEON_PCIE_TX_GART_START_LO,
> dev_priv->gart_vm_start);
A dma_addr_t (dev_priv->bus_pci_gart) is a long on at least some
systems. While we may know that it's 32 bits here, a cast will be
needed to avoid warnings.
ioctls where removing the root check introduces privelege escalation for
users with read access to the DRM device (at least):
- DRM_R128_INDIRECT
- DRM_RADEON_INDIRECT
--
Eric Anholt [EMAIL PROTECTED]
http://people.freebsd.org/~anholt/ [EMAIL PROTECTED]
---
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
