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

[Andrew Morton <akpm@osdl.org>]

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. */
_