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
> ---------------------------------------------------------
>
> 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
Hi,
Hollis Blanchard <[email protected]> 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 ------------------------------ [email protected]