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 */
+ }
+ 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.
*
Attachment:
pgppeyZogbtWu.pgp
Description: PGP signature