Messages in this thread Patch in this message | | | Date | Thu, 26 Aug 2004 03:14:14 -0700 | From | Andrew Morton <> | Subject | Re: 2.6.9-rcX cdrom.c is subject to "chaotic" behaviour |
| |
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. */ _ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |