[CHECKER][PATCH] cmpci user-pointer fix

[CHECKER][PATCH] cmpci user-pointer fix

[Hollis Blanchard]

[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]

Here's what the Stanford tool said:
---------------------------------------------------------
[BUG] at least bad programming practice. file_operations.write ->
cm_write -> trans_ac3. write can take tainted. write can take tainted
inputs. the pointer is vefied in cm_write

/home/junfeng/linux-2.5.63/sound/oss/cmpci.c:593:trans_ac3:
ERROR:TAINTED:593:593: dereferencing tainted ptr 'src' [Callstack:
/home/junfeng/linux-2.5.63/fs/read_write.c:307:vfs_write((tainted
1)(tainted 2)) ->
/home/junfeng/linux-2.5.63/fs/read_write.c:241:cm_write((tainted
1)(tainted 2)) ->
/home/junfeng/linux-2.5.63/sound/oss/cmpci.c:1662:trans_ac3((tainted
2))]

         unsigned long data;
         unsigned long *dst = (unsigned long *) dest;
         unsigned short *src = (unsigned short *)source;

         do {

Error --->
                 data = (unsigned long) *src++;
                 data <<= 12;                    // ok for 16-bit data
                 if (s->spdif_counter == 2 || s->spdif_counter == 3)
---------------------------------------------------------
[BUG] at least bad programming practice. file_operations.write ->
cm_write -> trans_ac3. write can take tainted. write can take tainted
inputs. the pointer is vefied in cm_write

/home/junfeng/linux-2.5.63/sound/oss/cmpci.c:593:trans_ac3:
ERROR:TAINTED:593:593: dereferencing tainted ptr 'src' [Callstack:
/home/junfeng/linux-2.5.63/fs/read_write.c:307:vfs_write((tainted
1)(tainted 2)) ->
/home/junfeng/linux-2.5.63/fs/read_write.c:241:cm_write((tainted
1)(tainted 2)) ->
/home/junfeng/linux-2.5.63/sound/oss/cmpci.c:1662:trans_ac3((tainted
2))]

         unsigned long data;
         unsigned long *dst = (unsigned long *) dest;
         unsigned short *src = (unsigned short *)source;

         do {

Error --->
                 data = (unsigned long) *src++;
                 data <<= 12;                    // ok for 16-bit data
                 if (s->spdif_counter == 2 || s->spdif_counter == 3)
                         data |= 0x40000000;     // indicate AC-3 raw 
data
---------------------------------------------------------

I believe the attached patch fixes it. cm_write was calling access_ok, 
but after that you must still access user space through the 
get/put/copy*_user functions. It should be safe to return -EFAULT at 
these points in cm_write, since there are other returns already in the 
code above and below that. Compile-tested only.

Junfeng, the checker seems to have missed the "*dst0++ = *src++;" bits 
at the bottom of the patch... or at least it wasn't in the mail I saw 
("4 potential user-pointer errors", 2 May 2003).

-- 
Hollis Blanchard
IBM Linux Technology Center

[-- Attachment #2: cmpci-userptr.diff --]
[-- Type: application/octet-stream, Size: 1916 bytes --]

--- linux-2.5.70/sound/oss/cmpci.c.orig	Sat May 24 19:00:00 2003
+++ linux-2.5.70/sound/oss/cmpci.c	Wed May 28 14:53:15 2003
@@ -580,15 +580,17 @@
 	spin_unlock_irqrestore(&s->lock, flags);
 }
 
-static void trans_ac3(struct cm_state *s, void *dest, const char *source, int size)
+static int trans_ac3(struct cm_state *s, void *dest, const char *source, int size)
 {
 	int   i = size / 2;
+	int err;
 	unsigned long data;
 	unsigned long *dst = (unsigned long *) dest;
 	unsigned short *src = (unsigned short *)source;
 
 	do {
-		data = (unsigned long) *src++;
+		if ((err = __get_user(data, src++)))
+			return err;
 		data <<= 12;			// ok for 16-bit data
 		if (s->spdif_counter == 2 || s->spdif_counter == 3)
 			data |= 0x40000000;	// indicate AC-3 raw data
@@ -605,6 +607,8 @@
 		if (s->spdif_counter == 384)
 			s->spdif_counter = 0;
 	} while (--i);
+
+	return 0;
 }
 
 static void set_adc_rate_unlocked(struct cm_state *s, unsigned rate)
@@ -1655,13 +1659,16 @@
 			continue;
 		}
 		if (s->status & DO_AC3_SW) {
+			int err;
+
 			// clip exceeded data, caught by 033 and 037
 			if (swptr + 2 * cnt > s->dma_dac.dmasize)
 				cnt = (s->dma_dac.dmasize - swptr) / 2;
-			trans_ac3(s, s->dma_dac.rawbuf + swptr, buffer, cnt);
+			if ((err = trans_ac3(s, s->dma_dac.rawbuf + swptr, buffer, cnt)))
+				return err;
 			swptr = (swptr + 2 * cnt) % s->dma_dac.dmasize;
 		} else if (s->status & DO_DUAL_DAC) {
-			int	i;
+			int	i, err;
 			unsigned long *src, *dst0, *dst1;
 
 			src = (unsigned long *) buffer;
@@ -1669,8 +1676,10 @@
 			dst1 = (unsigned long *) (s->dma_adc.rawbuf + swptr);
 			// copy left/right sample at one time
 			for (i = 0; i <= cnt / 4; i++) {
-				*dst0++ = *src++;
-				*dst1++ = *src++;
+				if ((err = __get_user(*dst0++, src++)))
+					return err;
+				if ((err = __get_user(*dst1++, src++)))
+					return err;
 			}
 			swptr = (swptr + cnt) % s->dma_dac.dmasize;
 		} else {

Re: [CHECKER][PATCH] cmpci user-pointer fix

[Junfeng Yang]

> ---------------------------------------------------------
>
> I believe the attached patch fixes it. cm_write was calling access_ok,
> but after that you must still access user space through the
> get/put/copy*_user functions. It should be safe to return -EFAULT at
> these points in cm_write, since there are other returns already in the
> code above and below that. Compile-tested only.

Thanks a lot for the fixes!

>
> Junfeng, the checker seems to have missed the "*dst0++ = *src++;" bits
> at the bottom of the patch... or at least it wasn't in the mail I saw
> ("4 potential user-pointer errors", 2 May 2003).

I deliberately surpressed 'redudant' error reports.  When the checker saw
multiple errors caused by the same user pointer in one function, it'll
only report one error for this user pointer in this function.  if I don't
do so, people tend to get bored by the 'repeated' messages, and leave
error reports in other functions uninspected. ;) I'll re-run the checker
soon with the suppression off.

-Junfeng

Re: [CHECKER][PATCH] cmpci user-pointer fix

[Raja R Harinath]

Hi,

Hollis Blanchard <hollisb@us.ibm.com> writes:
[snip]
> I believe the attached patch fixes it. cm_write was calling access_ok,
> but after that you must still access user space through the
> get/put/copy*_user functions. It should be safe to return -EFAULT at
> these points in cm_write, since there are other returns already in the
> code above and below that. Compile-tested only.
[snip]
> --- linux-2.5.70/sound/oss/cmpci.c.orig	Sat May 24 19:00:00 2003
> +++ linux-2.5.70/sound/oss/cmpci.c	Wed May 28 14:53:15 2003
> @@ -580,15 +580,17 @@
>  	spin_unlock_irqrestore(&s->lock, flags);
>  }
>  
> -static void trans_ac3(struct cm_state *s, void *dest, const char *source, int size)
> +static int trans_ac3(struct cm_state *s, void *dest, const char *source, int size)

Shouldn't 'source' get the new __user annotation, then:

  const char * __user source

IIRC.

>  {
>  	int   i = size / 2;
> +	int err;
>  	unsigned long data;
>  	unsigned long *dst = (unsigned long *) dest;
>  	unsigned short *src = (unsigned short *)source;

Likewise with 'src'.

- Hari
-- 
Raja R Harinath ------------------------------ harinath@cs.umn.edu