Re: bad cd features descriptor

[Arnaud Lacombe <lacombar%gmail.com@localhost>]

Hi,

On Thu, Jul 21, 2011 at 11:23 AM, Greg Troxel <gdt%ir.bbn.com@localhost> wrote:
>
> I have a USB stick with the "U3" misfeature, which has an emulated
> cdrom.  netbsd-5 crashes soon after this is inserted (when hald tries to
> get features/info, I think).  I looked at my crash dump and added sanity
> checking.  I was unaware of martin's commit:
>
>  revision 1.297
>  date: 2010/01/06 20:16:57;  author: martin;  state: Exp;  lines: +4 -2
>  branches:  1.297.2;
>  Some usb devices come with an internal emulated umass CD drive (containing
>  windows drivers). I have such a device that has 0 features - avoid wrappig
>  features_len to a very big unsiged 32bit number in this case.
>
> and while I think that commit is entirely sound there are more than just
> one input validation problem in the existing code.  I have the following
> diff to netbsd-5 that successfully complains about the device's failure
> to return something reasonable (0 features would ok, but there's an 8
> byte header that is also not returned) instead of crashing.
>
> In addition, I don't understand how the while loop about features_len
> being > 0xffff ever makes sense.  Clearly the point is to do another
> fetch of another buffer's worth, but it's the wrong test.  It cannot
> ever succeed, because features_len is checked to fit in 1024 bytes;
> between that and not understanding, I didn't change it.
>
> My change is essentially a bunch of sanity checks that error out on
> feature reading.
>
> Any objections to me committing this to current (and replacing the test
> in 1.297 with this)?
>
>
> --- cd.c.~1.283.4.3.~   2011-01-13 10:14:57.000000000 -0500
> +++ cd.c        2011-07-21 11:21:36.000000000 -0400
> @@ -3033,34 +3033,75 @@ mmc_getdiscinfo(struct scsipi_periph *pe
>
>                features_len = _4btol(gc->data_len);
>
> +               /*
> +                * features_len is the length of a struct
> +                * scsipi_get_conf_data, which has a header (8 bytes)
> +                * followed by some number of features.
> +                */
> +
> +               if (features_len < 8 || features_len > feat_tbl_len) {
> +                       printf("mmc_getdiscinfo: bad features_len %d (%d)\n",
> +                              features_len, feat_tbl_len);
> +                       goto done_features;
> +               }
> +
> +               /*
> +                * pos is the offset after the 8-byte header, and fpos
> +                * is a pointer to the feature being processed.
> +                */
>                pos  = 0;
>                fpos = &gc->feature_desc[0];
> -               while (pos < features_len - 4) {
> +               /* Check for enough space for a feature header. */
> +               while (pos + 8 + 4 < features_len) {
>                        gcf = (struct scsipi_get_conf_feature *) fpos;
>
>                        feature     = _2btol(gcf->featurecode);
>                        feature_cur = gcf->flags & 1;
>                        feature_len = gcf->additional_length;
>
> +                       /* Check that this feature fits, and is long-aligned. 
> */
> +                       if (feature_len < 0 ||
> +                           8 + pos + 4 + feature_len > feat_tbl_len ||
> +                           (feature_len & 3) != 0) {
> +                               printf("feature %d bad feature_len %d (pos %d 
> feat_tbl_len %d)\n",
> +                                      feature, feature_len, pos, 
> feat_tbl_len);
> +                               goto done_features;
> +                       }
> +
> +                       /* XXX How can this not take feature_len? */
>                        mmc_process_feature(mmc_discinfo,
>                                            feature, feature_cur,
>                                            gcf->feature_dependent);
>
> -                       last_feature = MAX(last_feature, feature);
> -#ifdef DIAGNOSTIC
> -                       /* assert((feature_len & 3) == 0); */
> -                       if ((feature_len & 3) != 0) {
> -                               printf("feature %d having length %d\n",
> -                                       feature, feature_len);
> +                       /*
> +                        * Support a subsequent get_conf_feature
> +                        * starting (in theory) after the one we read.
> +                        * XXX Why is this not just an assignment?  How
> +                        * can we be sure this won't loop?
> +                        */
> +                       if (last_feature < feature) {
> +                               printf("misordered last_feature %d feature 
> %d\n",
> +                                      last_feature, feature);
> +                               goto done_features;
>                        }
> -#endif
> +                       last_feature = MAX(last_feature, feature);
>
>                        pos  += 4 + feature_len;
>                        fpos += 4 + feature_len;
> +
> +#ifdef DIAGNOSTIC
> +                       KASSERT(pos + 8 <= feat_tbl_len);
> +#endif /* DIAGNOSTIC */
on a pure stylish pov., what's the point of these #ifdef ... #endif ?
AFAIK, KASSERT() should already be conditionally defined whether or
not DIAGNOSTIC.

 - Arnaud

> +               }
> +               if (pos + 8 != features_len) {
> +                       printf("excess feature space pos %d features_len 
> %d\n",
> +                              pos, features_len);
>                }
>                /* unlikely to ever grow past our 1kb buffer */
>        } while (features_len >= 0xffff);
>
> +done_features:
> +
>        /*
>         * Fixup CD-RW drives that are on crack.
>         *
>