Bug 1803

Summary: Security issue: insufficient locking checks in DRM code
Product: xorg Reporter: Stefan Dirsch <sndirsch>
Component: Server/Ext/GLXAssignee: Default DRI bug account <dri-devel>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: high CC: dberkholz, eich, naimaathar.na
Version: git   
Hardware: Other   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
drm_lock.diff none

Description Stefan Dirsch 2004-11-09 05:59:51 UTC 
http://www.mail-archive.com/dri-devel%40lists.sourceforge.net/msg20254.html 
 
Michel DÃnzer wrote:  
On Mon, 2004-11-01 at 14:21 +0100, Thomas HellstrÃm wrote: 
   
  
Hmm, correct me If I'm wrong, but after a brief check in the code, it 
seems like the current _DRM_LOCK_IS_HELD() used in  dma buffer 
submission IOCTLS just checks that the lock is indeed held, but not if 
it is held by the current caller. Thus any authorized client should be 
able to sneek in DMA commands while the lock is held by another client 
or the X server. -> potential system crash. 
     
  
Hence _DRM_LOCK_IS_HELD() always seems to be (supposed to be) 
accompanied by another test that verifies the ownership. 
 
   
 Michael,  
  
 I just checked i830_dma.c, i915_dma.c and via_dma.c, and _DRM_LOCK_IS_HELD() 
is used without such a test, AFAICT. 
  
 The correct macro to call seems to be 
 LOCK_TEST_WITH_RETURN() 
 which does incorporate such a test. 
  
 In fact, the use of _DRM_LOCK_IS_HELD() here should allow malfunctioning or 
malicious SMP dri clients to modify internal drm data structures and DMA 
ring-buffers simultaneously?  
  
 /Thomas
Comment 1 Stefan Dirsch 2004-11-09 06:01:46 UTC 
Has this already been adressed? This issue has been reported to vendor-sec. 
I'll attach an according patch, which I didn't test yet, I must admit.
Comment 2 Stefan Dirsch 2004-11-09 06:02:23 UTC 
Created attachment 1250 [details] [review]
drm_lock.diff
Comment 3 Dave Airlie 2004-11-11 03:10:50 UTC 
I've applied this patch to the drm in CVS, 

I'll push to Linus, I'm not sure if it is a big security issue, but you never
know  I suppose, do vendors use the drm shipped with Xorg or with the kernel?
Comment 4 Stefan Dirsch 2004-11-11 03:29:11 UTC 
Thanks, Dave. Which CVS is this? X.Org? I cannot speak for other vendors, but 
SuSE currently uses the DRM shipped with X.Org.
Comment 5 Dave Airlie 2004-11-11 03:43:05 UTC 
I've commited it to the drm CVS tree, which will eventually get into Xorg for
the next major release, if there is a need to patch it into Xorg for 6.8.2 then
we should propose it via the Xorg bug fix method..

If SuSE are shipping a 2.6 kernel I'd recommend shipping the drm from the
kernel, the DRM CVS tree is development tree mainly now, and I pass the
stablised patches to Linus and probably the next Xorg release will just use a
stable release similiar to what is in the kernel..
Comment 6 Stefan Dirsch 2004-11-11 03:46:21 UTC 
Looks like I've found it in DRI CVS (drm module): 
 
./linux-core/i810_dma.c 
./linux-core/i830_dma.c 
./linux-core/i830_irq.c 
./shared-core/i915_dma.c 
./shared-core/i915_irq.c
Comment 7 Donnie Berkholz 2004-11-15 00:29:28 UTC 
Gentoo has historically provided and recommended DRM CVS snapshots, but there
haven't been any lately with all the stuff going on there. There's also the
option to use the kernel DRM.
Comment 8 Dave Airlie 2004-12-30 23:49:30 UTC 
These changes have just appeared in Linus tree, so I'm closing this bug.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.