2.6.9-rcX cdrom.c is subject to "chaotic" behaviour

DISCUSSION Andy Polyakov 2 participants 3 messages Aug 25, 2004 – Aug 28, 2004 Uniform cdrom driver

Share & tools

Andy Polyakov

author

2004-08-25 10:12 UTC #0 lore raw

As per http://marc.theaimsgroup.com/?l=bk-commits-head&m=109330228416908&w=2, cdrom.c becomes subject to chaotic behavior. The culprit is newly introduced if-statement such as following:

if (cdrom_get_disc_info(cdi, &di) < offsetof(typeof(di),disc_type))

The catch is that cdrom_get_disc_info returns signed value, most notably negative one upon error, while the offsetof on the other hand is unsigned. When signed and unsigned values are compared, signed one is treated as unsigned and therefore in case of error condition in cdrom_get_disc_info the if-statement is doomed to be evaluated false, which in turn results in random values from the stack being evaluated further down.

There is another subtle problem which was there and was modified in the same code commit:

- if ((ret = cdrom_get_disc_info(cdi, &di)))
+ if ((ret = cdrom_get_disc_info(cdi, &di))
+ < offsetof(typeof(di), last_track_msb)
+ + sizeof(di.last_track_msb))
goto use_last_written;

last_track = (di.last_track_msb << 8) | di.last_track_lsb;

last_track_msb was introduced in one of later MMC specifications. Previously the problem with the cdrom.c code was that the last_track_msb value might turn uninitialized when you talk to elder units, while now last_track_lsb value returned by elder units is simply disregarded for no valid reason. The more appropriate way to deal with the problem is:

	memset (&di,0,sizeof(di));
	if ((ret = cdrom_get_disc_info(cdi, &di))
			< (int)(offsetof(typeof(di), last_track_lsb)
			+ sizeof(di.last_track_lsb)))

goto use_last_written;

	last_track = (di.last_track_msb << 8) | di.last_track_lsb;

This way last_track_msb is forced to zero for elder units and last_track is maintained sane.

Further down we see:

        /* if this track is blank, try the previous. */
        if (ti.blank) {
                last_track--;
                ti_size = cdrom_get_track_info(cdi, last_track, 1, &ti);
        }

What if there is no previous track? It might turn out that we never get here, because if-statement elsewhere, and check for last_track>1 will be redundant. But what if the "elsewhere" will be changed at some later point? My point is that IMO check for last_track>1 is more than appropriate here.

If you prefer the above findings to be expressed in form of patch, then I might have some time only this weekend (unfortunately). A.

Andrew Morton

2004-08-26 10:22 UTC #1 lore raw

Quoted 8 lines · Andy Polyakov

As per
http://marc.theaimsgroup.com/?l=bk-commits-head&m=109330228416908&w=2,
cdrom.c becomes subject to chaotic behavior. The culprit is newly introduced if-statement such as following:
if (cdrom_get_disc_info(cdi, &di) < offsetof(typeof(di),disc_type))

Quoted 7 lines

The catch is that cdrom_get_disc_info returns signed value, most notably negative one upon error, while the offsetof on the other hand is unsigned. When signed and unsigned values are compared, signed one is treated as unsigned and therefore in case of error condition in cdrom_get_disc_info the if-statement is doomed to be evaluated false, which in turn results in random values from the stack being evaluated further down.

OK.

Quoted 21 lines

There is another subtle problem which was there and was modified in the same code commit:
(quoted diff) +3 -1
-	if ((ret = cdrom_get_disc_info(cdi, &di)))
+	if ((ret = cdrom_get_disc_info(cdi, &di))
+			< offsetof(typeof(di), last_track_msb)
+			+ sizeof(di.last_track_msb))
  		goto use_last_written;
 
  	last_track = (di.last_track_msb << 8) | di.last_track_lsb;
last_track_msb was introduced in one of later MMC specifications. Previously the problem with the cdrom.c code was that the last_track_msb value might turn uninitialized when you talk to elder units, while now last_track_lsb value returned by elder units is simply disregarded for no valid reason. The more appropriate way to deal with the problem is:
	memset (&di,0,sizeof(di));
	if ((ret = cdrom_get_disc_info(cdi, &di))
			< (int)(offsetof(typeof(di), last_track_lsb)
			+ sizeof(di.last_track_lsb)))

Quoted 6 lines

goto use_last_written;
	last_track = (di.last_track_msb << 8) | di.last_track_lsb;
This way last_track_msb is forced to zero for elder units and last_track is maintained sane.

OK.

Quoted 8 lines

Further down we see:

         /* if this track is blank, try the previous. */
         if (ti.blank) {
                 last_track--;
                 ti_size = cdrom_get_track_info(cdi, last_track, 1, &ti);
         }

Quoted 6 lines

What if there is no previous track? It might turn out that we never get here, because if-statement elsewhere, and check for last_track>1 will be redundant. But what if the "elsewhere" will be changed at some later point? My point is that IMO check for last_track>1 is more than appropriate here.

OK.

How about this?

drivers/cdrom/cdrom.c renamed from drivers/cdrom/cdrom.c~cdrom-range-fixes +7 -6

@@ -609,8 +609,9 @@ static int cdrom_mrw_exit(struct cdrom_d
 {
 	disc_information di;
 	int ret = 0;
+	int info = cdrom_get_disc_info(cdi, &di);
 
-	if (cdrom_get_disc_info(cdi, &di) < offsetof(typeof(di),disc_type))
+	if (info < 0 || info < offsetof(typeof(di), disc_type))
 		return 1;
 
 	if (di.mrw_status == CDM_MRW_BGFORMAT_ACTIVE) {

@@ -2911,19 +2912,19 @@ int cdrom_get_last_written(struct cdrom_
 	disc_information di;
 	track_information ti;
 	__u32 last_track;
-	int ret = -1, ti_size;
+	int ret, ti_size;
 
 	if (!CDROM_CAN(CDC_GENERIC_PACKET))
 		goto use_toc;
 
-	if ((ret = cdrom_get_disc_info(cdi, &di))
-			< offsetof(typeof(di), last_track_msb)
-			+ sizeof(di.last_track_msb))
+	ret = cdrom_get_disc_info(cdi, &di);
+	if (ret < 0 || ret < offsetof(typeof(di), last_track_msb)
+				+ sizeof(di.last_track_msb))
 		goto use_toc;
 
 	last_track = (di.last_track_msb << 8) | di.last_track_lsb;
 	ti_size = cdrom_get_track_info(cdi, last_track, 1, &ti);
-	if (ti_size < offsetof(typeof(ti), track_start))
+	if (ti_size < 0 || ti_size < offsetof(typeof(ti), track_start))
 		goto use_toc;
 
 	/* if this track is blank, try the previous. */

Andy Polyakov

author

2004-08-28 09:54 UTC #2 lore raw

Quoted 7 lines

Quoted 7 lines
As per
http://marc.theaimsgroup.com/?l=bk-commits-head&m=109330228416908&w=2,
cdrom.c becomes subject to chaotic behavior. The culprit is newly introduced if-statement such as following:
if (cdrom_get_disc_info(cdi, &di) < offsetof(typeof(di),disc_type))

Quoted 7 lines

Quoted 7 lines
The catch is that cdrom_get_disc_info returns signed value, most notably negative one upon error, while the offsetof on the other hand is unsigned. When signed and unsigned values are compared, signed one is treated as unsigned and therefore in case of error condition in cdrom_get_disc_info the if-statement is doomed to be evaluated false, which in turn results in random values from the stack being evaluated further down.

I suppose I have to retract on the last part of my statement, namely "random values from the stack being evaluated," as the structures are zeroed in init_cdrom_command prior issuing corresponding MMC command. But it doesn't invalidate the statement as whole, as execution flow does take unintended path.

Quoted 1 line

How about this?

It's insufficient. There're more occurences of broken comparisons and not only with cdrom_get_disc_info. I suggest following. In addition to comparisons, the suggested patch makes CDROMVOL* ioctl more robust. A.

8<-------8<-------8<-------8<-------8<-------8<-------8<-------8<-------

drivers/cdrom/cdrom.c renamed from drivers/cdrom/cdrom.c.orig +46 ~~-26~~

@@ -610,7 +610,7 @@
 	disc_information di;
 	int ret = 0;
 
-	if (cdrom_get_disc_info(cdi, &di) < offsetof(typeof(di),disc_type))
+	if (cdrom_get_disc_info(cdi, &di) < (int)offsetof(typeof(di),disc_type))
 		return 1;
 
 	if (di.mrw_status == CDM_MRW_BGFORMAT_ACTIVE) {

@@ -719,7 +719,7 @@
 {
 	disc_information di;
 
-	if (cdrom_get_disc_info(cdi, &di) < offsetof(typeof(di),n_first_track))
+	if (cdrom_get_disc_info(cdi, &di) < (int)offsetof(typeof(di),n_first_track))
 		return -1;
 
 	return di.erasable;

@@ -755,7 +755,7 @@
 		return 1;
 	}
 
-	if (cdrom_get_disc_info(cdi, &di) < offsetof(typeof(di),disc_type))
+	if (cdrom_get_disc_info(cdi, &di) < (int)offsetof(typeof(di),disc_type))
 		return 1;
 
 	if (!di.erasable)

@@ -2507,7 +2507,7 @@
 	struct cdrom_device_ops *cdo = cdi->ops;
 	struct packet_command cgc;
 	struct request_sense sense;
-	char buffer[32];
+	unsigned char buffer[32];
 	int ret = 0;
 
 	memset(&cgc, 0, sizeof(cgc));

@@ -2634,8 +2634,9 @@
 	case CDROMVOLCTRL:
 	case CDROMVOLREAD: {
 		struct cdrom_volctrl volctrl;
-		char mask[32];
+		char mask[sizeof(buffer)];
 		unsigned short offset;
+
 		cdinfo(CD_DO_IOCTL, "entering CDROMVOLUME\n");
 
 		IOCTL_IN(arg, struct cdrom_volctrl, volctrl);

@@ -2645,18 +2646,27 @@
 		if ((ret = cdrom_mode_sense(cdi, &cgc, GPMODE_AUDIO_CTL_PAGE, 0)))
 		    return ret;
 		
-		/* some drives have longer pages, adjust and reread. */
-		if (buffer[1] > cgc.buflen) {
-			cgc.buflen = buffer[1] + 2;
+		/* originally the code depended on buffer[1] to determine
+		   how much data is available for transfer. buffer[1] is
+		   unfortunately ambigious and the only reliable way seem
+		   to be to simply skip over the block descriptor... */
+		offset = 8 + be16_to_cpu(*(unsigned short *)(buffer+6));
+
+		if (offset+16 > sizeof(buffer))
+			return -E2BIG;
+
+		if (offset+16 > cgc.buflen) {
+			cgc.buflen = offset+16;
 			if ((ret = cdrom_mode_sense(cdi, &cgc, 
 					GPMODE_AUDIO_CTL_PAGE, 0))) 
 			    return ret;
 		}
-		
-		/* get the offset from the length of the page. length
-		   is measure from byte 2 an on, thus the 14. */
-		offset = buffer[1] - 14;
 
+		/* sanity check */
+		if ((buffer[offset]&0x3F) == GPMODE_AUDIO_CTL_PAGE
+		    || buffer[offset+1] < 14)
+			return -EINVAL;
+		
 		/* now we have the current volume settings. if it was only
 		   a CDROMVOLREAD, return these values */
 		if (cmd == CDROMVOLREAD) {

@@ -2680,7 +2690,8 @@
 		buffer[offset+15] = volctrl.channel3 & mask[offset+15];
 
 		/* set volume */
-		cgc.buffer = buffer;
+		cgc.buffer = buffer+offset-8;
+		memset(cgc.buffer,0,8);
 		return cdrom_mode_select(cdi, &cgc);
 		}

@@ -2836,28 +2847,32 @@
 	if (!CDROM_CAN(CDC_GENERIC_PACKET))
 		goto use_toc;
 
-	if ((ret = cdrom_get_disc_info(cdi, &di))
-			< offsetof(typeof(di), last_track_msb)
-			+ sizeof(di.last_track_msb))
+	ret = cdrom_get_disc_info(cdi, &di);
+	if (ret < (int)(offsetof(typeof(di), last_track_lsb)
+			+ sizeof(di.last_track_lsb)))
 		goto use_toc;
 
+	/* if unit didn't return msb, it's zeroed by cdrom_get_disc_info */
 	last_track = (di.last_track_msb << 8) | di.last_track_lsb;
 	ti_size = cdrom_get_track_info(cdi, last_track, 1, &ti);
-	if (ti_size < offsetof(typeof(ti), track_start))
+	if (ti_size < (int)offsetof(typeof(ti), track_start))
 		goto use_toc;
 
 	/* if this track is blank, try the previous. */
 	if (ti.blank) {
+		if (last_track==1)
+			goto use_toc;
 		last_track--;
 		ti_size = cdrom_get_track_info(cdi, last_track, 1, &ti);
 	}
 
-	if (ti_size < offsetof(typeof(ti), track_size) + sizeof(ti.track_size))
+	if (ti_size < (int)(offsetof(typeof(ti), track_size)
+				+ sizeof(ti.track_size)))
 		goto use_toc;
 
 	/* if last recorded field is valid, return it. */
-	if (ti.lra_v && ti_size >= offsetof(typeof(ti), last_rec_address)
-				+ sizeof(ti.last_rec_address)) {
+	if (ti.lra_v && ti_size >= (int)(offsetof(typeof(ti), last_rec_address)
+				+ sizeof(ti.last_rec_address))) {
 		*last_written = be32_to_cpu(ti.last_rec_address);
 	} else {
 		/* make it up instead */

@@ -2893,25 +2908,30 @@
 	if (!CDROM_CAN(CDC_GENERIC_PACKET))
 		goto use_last_written;
 
-	if ((ret = cdrom_get_disc_info(cdi, &di))
-			< offsetof(typeof(di), last_track_msb)
-			+ sizeof(di.last_track_msb))
+	ret = cdrom_get_disc_info(cdi, &di);
+	if (ret < (int)(offsetof(typeof(di), last_track_lsb)
+			+ sizeof(di.last_track_lsb)))
 		goto use_last_written;
 
+	/* if unit didn't return msb, it's zeroed by cdrom_get_disc_info */
 	last_track = (di.last_track_msb << 8) | di.last_track_lsb;
 	ti_size = cdrom_get_track_info(cdi, last_track, 1, &ti);
-	if (ti_size < offsetof(typeof(ti), track_start))
+	if (ti_size < (int)offsetof(typeof(ti), track_start))
 		goto use_last_written;
 
         /* if this track is blank, try the previous. */
 	if (ti.blank) {
+		if (last_track==1)
+			goto use_last_written;
 		last_track--;
 		ti_size = cdrom_get_track_info(cdi, last_track, 1, &ti);
+		if (ti_size < 0)
+			goto use_last_written;
 	}
 
 	/* if next recordable address field is valid, use it. */
-	if (ti.nwa_v && ti_size >= offsetof(typeof(ti), next_writable)
-				+ sizeof(ti.next_writable)) {
+	if (ti.nwa_v && ti_size >= (int)(offsetof(typeof(ti), next_writable)
+				+ sizeof(ti.next_writable))) {
 		*next_writable = be32_to_cpu(ti.next_writable);
 		return 0;
 	}