[PATCH] fix a bug in ioctl(CDROMREADAUDIO) in cdrom.c in 2.2.18

[PATCH] fix a bug in ioctl(CDROMREADAUDIO) in cdrom.c in 2.2.18

[Jani Jaakkola]

Using ioctl(CDROMREADAUDIO) with nframes argument being larger than 8 and
not divisible by 8 causes kernel to read and return more audio data than
was requested. This is bad since it clobbers up processes memory
(I noticed this when my patched cdparanoia segfaulted).

This _might_ also have a security impact, since it could be used to
overwrite memory which the user should not have write access with
cdrom audio data. (_might_ since I do not know the exact semantics of
__copy_to_user() and I am too lazy to check them out. The attacker needs
access to cdrom device with audio cdrom in drive, preferably with a
custom made audio cd).

I have not checked if the same bug is also present in 2.4 kernels.

If you have any comments, please Cc: them to me, since I am not present in
the list.

Here is a trivial patch against drivers/cdrom/cdrom.c of kernel 2.2.18:

--- cdrom.c.orig	Wed Mar 14 13:15:13 2001
+++ cdrom.c	Wed Mar 14 15:42:19 2001
@@ -1946,6 +1946,7 @@
 			ra.buf += (CD_FRAMESIZE_RAW * frames);
 			ra.nframes -= frames;
 			lba += frames;
+			if (frames>ra.nframes) frames=ra.nframes;
 		}
 		kfree(cgc.buffer);
 		return ret;

RE: [PATCH] fix a bug in ioctl(CDROMREADAUDIO) in cdrom.c in 2.2

[Pierre Etchemaite]

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


Le 14-Mar-2001, Jani Jaakkola écrivait :
> 
> Using ioctl(CDROMREADAUDIO) with nframes argument being larger than 8 and
> not divisible by 8 causes kernel to read and return more audio data than
> was requested. This is bad since it clobbers up processes memory
> (I noticed this when my patched cdparanoia segfaulted).
 
Same thing for 2.4.2.

Is my allocation loop "over engineering", or just plain bad thing to do ?


Regards,
Pierre.


-- 
Linux blade.concept-micro.com 2.4.3-pre4 #1 Wed Mar 14 22:19:14 CET 2001 i686 unknown
  2:04pm  up 11:29,  4 users,  load average: 2.66, 2.80, 2.26


[-- Attachment #2: myreadaudio.patch --]
[-- Type: application/octet-stream, Size: 1482 bytes --]

--- linux-2.4.2/drivers/cdrom/cdrom.c	Wed Mar 14 22:15:52 2001
+++ linux/drivers/cdrom/cdrom.c	Wed Mar 14 22:16:25 2001
@@ -1985,7 +1985,7 @@
 		}
 	case CDROMREADAUDIO: {
 		struct cdrom_read_audio ra;
-		int lba;
+		int lba, frames;
 
 		IOCTL_IN(arg, struct cdrom_read_audio, ra);
 
@@ -2002,8 +2002,13 @@
 		if (lba < 0 || ra.nframes <= 0)
 			return -EINVAL;
 
-		if ((cgc.buffer = (char *) kmalloc(CD_FRAMESIZE_RAW, GFP_KERNEL)) == NULL)
-			return -ENOMEM;
+		frames = ra.nframes > 8 ? 8 : ra.nframes;
+
+                while((cgc.buffer = (char *) kmalloc(CD_FRAMESIZE_RAW * frames,
 GFP_KERNEL)) == NULL) {
+		  frames = frames >> 1;
+		  if (!frames) 
+		    return -ENOMEM;
+                };
 
 		if (!access_ok(VERIFY_WRITE, ra.buf, ra.nframes*CD_FRAMESIZE_RAW)) {
 			kfree(cgc.buffer);
@@ -2011,12 +2016,16 @@
 		}
 		cgc.data_direction = CGC_DATA_READ;
 		while (ra.nframes > 0) {
-			ret = cdrom_read_block(cdi, &cgc, lba, 1, 1, CD_FRAMESIZE_RAW);
-			if (ret) break;
-			__copy_to_user(ra.buf, cgc.buffer, CD_FRAMESIZE_RAW);
-			ra.buf += CD_FRAMESIZE_RAW;
-			ra.nframes--;
-			lba++;
+		        if (frames > ra.nframes)
+			        frames = ra.nframes;
+			ret = cdrom_read_block(cdi, &cgc, lba, frames, 1, CD_FRAMESIZE_RAW);
+			if (ret)
+				break;
+			__copy_to_user(ra.buf, cgc.buffer,
+				       CD_FRAMESIZE_RAW * frames);
+			ra.buf += (CD_FRAMESIZE_RAW * frames);
+			ra.nframes -= frames;
+			lba += frames;
 		}
 		kfree(cgc.buffer);
 		return ret;

Re: [PATCH] fix a bug in ioctl(CDROMREADAUDIO) in cdrom.c in 2.2

[David Mansfield]

Pierre Etchemaite wrote:
> 
> Le 14-Mar-2001, Jani Jaakkola écrivait :
> >
> > Using ioctl(CDROMREADAUDIO) with nframes argument being larger than 8 and
> > not divisible by 8 causes kernel to read and return more audio data than
> > was requested. This is bad since it clobbers up processes memory
> > (I noticed this when my patched cdparanoia segfaulted).
> 
> Same thing for 2.4.2.
> 
> Is my allocation loop "over engineering", or just plain bad thing to do ?
> 

I've been running this (or close: my version tries 8 frames, then jumps
immediately to 1, without trying 4 and 2 in between if the kmalloc
fails) since it was changed.  Without such a patch, my CDDA read speed
drops to 25% the original rate.  You also have the fix that started the
thread!

Jens (cdrom maintainer) said he was working on a more elegant solution,
but to me, such a simple fix as yours should go in the kernel in the
meantime.  Jens?

-- 
David Mansfield                                           (718) 963-2020
david@ultramaster.com
Ultramaster Group, LLC                               www.ultramaster.com

Re: [PATCH] fix a bug in ioctl(CDROMREADAUDIO) in cdrom.c in 2.2

[Jens Axboe]

On Fri, Mar 16 2001, David Mansfield wrote:
> > Same thing for 2.4.2.
> > 
> > Is my allocation loop "over engineering", or just plain bad thing to do ?
> > 
> 
> I've been running this (or close: my version tries 8 frames, then jumps
> immediately to 1, without trying 4 and 2 in between if the kmalloc
> fails) since it was changed.  Without such a patch, my CDDA read speed
> drops to 25% the original rate.  You also have the fix that started the
> thread!
> 
> Jens (cdrom maintainer) said he was working on a more elegant solution,
> but to me, such a simple fix as yours should go in the kernel in the
> meantime.  Jens?

I haven't integrated it yet, because of the vm printing memory
allocations errors. Which sort of destroys the idea of doing "clever"
allocations like this.

-- 
Jens Axboe