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

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

[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))

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.

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

[Andrew Morton]

Andy Polyakov <appro@fy.chalmers.se> wrote:
>
> 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.

OK.

> 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.

OK.

> 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.
> 

OK.

How about this?

--- 25/drivers/cdrom/cdrom.c~cdrom-range-fixes	2004-08-26 03:06:40.533279808 -0700
+++ 25-akpm/drivers/cdrom/cdrom.c	2004-08-26 03:12:17.208097456 -0700
@@ -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. */
_

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

[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))
> >
> > 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.

> 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.orig	Tue Aug 24 18:54:41 2004
+++ ./drivers/cdrom/cdrom.c	Sat Aug 28 11:08:43 2004
@@ -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;
 	}