From: Crystal Kolipe Subject: /sbin/scsi bugfixes To: tech@openbsd.org Date: Thu, 15 May 2025 13:00:52 -0300 The /sbin/scsi utility has some, (very), broken logic in the mode page printing code, when printing raw, unformatted hex bytes. I discovered this whilst working on the read-only softraid patches, but the bugs in /sbin/scsi are not specific to this. From mode_edit() in scsi.c: mode_sense(fd, data, sizeof(data), pagectl, page); /* Skip over the block descriptors. */ mh = (struct mode_header *)data; mph = (struct mode_page_header *)(((char *)mh) + sizeof(*mh) + mh->bdl); mode_pars = (char *)mph + sizeof(*mph); if (!fmt) { for (i = 0; i < mh->mdl; i++) { printf("%02x%c",mode_pars[i], (((i + 1) % 8) == 0) ? '\n' : ' '); } This loop is intended to print SCSI mode page data as hex. The data returned by mode_sense() in 'data' has the following format: 4 byte header variable size block descriptors 2 byte mode page header mode page data If page 0x3f is requested, multiple pages are concatenated with their 2 byte headers, so we get: 4 byte header variable size block descriptors 2 byte mode page header mode page data 2 byte mode page header mode page data ... The for loop loops for the number of bytes specified in the mode sense header. This value includes the length of not only the mode page data that follows, but also the last three bytes of that four byte mode sense header itself as well as the block descriptors. In other words, the value is the total data length in 'data' minus one. The existing printing logic starts printing byte values after the 2-byte mode _page_ header, (struct mode_page_header), that follows the 4-byte mode sense header, (struct mode_header). So it seems that the intention of the author was to dump _only_ the actual mode page data bytes, (variable names in earlier version of the code support this theory as well). In this case, we just have an excess five bytes trailing the useful output. However, this intention to skip the 2-byte mode page header breaks down when requesting mode page 0x3f, which represents _all_ supported mode pages. In this case, the data returned is the concatenated octet stream of: 2 bytes mode page header, variable-size mode page data, (repeat) ... So the hex dump output in this case is also 'off by five', but the mode page header for the first mode page is missing, (because it was skipped), whereas those for the second and subsequent mode pages are present in the output. This bug seems to was introduced to the code in 1995, before the original revision in the OpenBSD cvs. Note that the program always uses the 6-byte mode sense opcode, (1A), and never the 10-byte version, (5A), so at least we don't have to concern ourselves further with that. Additionally, the existing code skips over any returned block descriptors, but doesn't subtract that number of bytes from the loop test condition, so it ends up reading extra meaningless bytes. Proposed fix has two parts: 1. Print the correct number of bytes for all mode page requests. 2. Disable skipping the two-byte mode page header if requesting page 0x3f. Whilst it seems unlikely that the output is being using in scripts, part 2 maintains the existing byte offsets for displays of single mode pages, and makes the display consistent when requesting all pages. --- scsi.c.dist Sun Dec 4 23:50:47 2022 +++ scsi.c Thu May 15 15:02:58 2025 @@ -804,10 +804,23 @@ */ mh = (struct mode_header *)data; mph = (struct mode_page_header *)(((char *)mh) + sizeof(*mh) + mh->bdl); - mode_pars = (char *)mph + sizeof(*mph); + /* + * If the data length was just block descriptors and the three header + * bytes, we're done. + */ + if (mh->bdl + 3 >= mh->mdl) + return ; + + /* + * Skip mode page header unless we are requesting page 0x3f, (all pages) + */ + + mode_pars = (char *)mph + (((page & 0x3f) == 0x3f) ? 0 : sizeof(*mph)); + if (!fmt) { - for (i = 0; i < mh->mdl; i++) { + for (i = 0; i < (mh->mdl - mh->bdl - + (((page & 0x3f) == 0x3f) ? 3 : 5)); i++) { printf("%02x%c",mode_pars[i], (((i + 1) % 8) == 0) ? '\n' : ' '); }