Re: [CHECKER] Probable security holes in 2.6.5

[Andrea Arcangeli <andrea@suse.de>]

On Fri, Apr 16, 2004 at 11:54:06AM -0700, Chris Wright wrote:
> * Ken Ashcraft (ken@coverity.com) wrote:
> > [BUG]
> > /home/kash/linux/linux-2.6.5/drivers/char/drm/i810_dma.c:1276:i810_dma_mc: ERROR:TAINT: 1267:1276:Using user value "((mc).idx * 4)" without first performing bounds checks [SOURCE_MODEL=(lib,copy_from_user,user,taintscalar)] [PATH= "(*((*dev).lock).hw_lock).lock & -2147483648 == 0" on line 1271 is false => "copy_from_user != 0" on line 1267 is false]    
> > 	u32 *hw_status = dev_priv->hw_status_page;
> > 	drm_i810_sarea_t *sarea_priv = (drm_i810_sarea_t *)
> > 		dev_priv->sarea_priv;
> > 	drm_i810_mc_t mc;
> > 
> > Start --->
> > 	if (copy_from_user(&mc, (drm_i810_mc_t *)arg, sizeof(mc)))
> > 		return -EFAULT;
> > 
> > 
> > 	if (!_DRM_LOCK_IS_HELD(dev->lock.hw_lock->lock)) {
> > 		DRM_ERROR("i810_dma_mc called without lock held\n");
> > 		return -EINVAL;
> > 	}
> > 
> > Error --->
> > 	i810_dma_dispatch_mc(dev, dma->buflist[mc.idx], mc.used,
> > 		mc.last_render );
> > 
> > 	atomic_add(mc.used, &dev->counts[_DRM_STAT_SECONDARY]);
> 
> Looks like a possible bug.  Index shouldn't go off end of buflist.
> Perhaps verifying it's below buf_count would do it.  Patch below.
> 
> thanks,
> -chris
> -- 
> Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net
> 
> ===== drivers/char/drm/i810_dma.c 1.31 vs edited =====
> --- 1.31/drivers/char/drm/i810_dma.c	Mon Apr 12 10:54:26 2004
> +++ edited/drivers/char/drm/i810_dma.c	Fri Apr 16 11:46:32 2004
> @@ -1275,6 +1275,9 @@
>  		return -EINVAL;
>  	}
>  
> +	if (mc.idx >= dma->buf_count)
> +		return -EINVAL;
> +
>  	i810_dma_dispatch_mc(dev, dma->buflist[mc.idx], mc.used,
>  		mc.last_render );

this is wrong, idx is signed, so you've to check for negative values
too. Credit for noticing this doesn't belong to me though.

Could you just in case review the other fixes too for other potential
errors like this? thanks.

this is the correct fix for 2.6 (and it seems 2.4 too):

--- linux/drivers/char/drm/i810_dma.c	Mon Apr 12 10:54:26 2004
+++ linux/drivers/char/drm/i810_dma.c	Fri Apr 16 11:46:32 2004
@@ -1275,6 +1275,9 @@
 		return -EINVAL;
 	}
 
+	if (mc.idx >= dma->buf_count || mc.idx < 0)
+		return -EINVAL;
+
 	i810_dma_dispatch_mc(dev, dma->buflist[mc.idx], mc.used,
 		mc.last_render );