From: Crystal Kolipe <kolipe.c@exoticsilicon.com>
Subject: /sbin/scsi bugfixes
To: tech@openbsd.org
Date: Thu, 12 Jun 2025 10:52:32 -0300

Any opinions on this from last month?

The existing mode page printing code is fairly broken and the patch is quite
straightforward.

On Thu, May 15, 2025 at 01:00:52PM -0300, Crystal Kolipe wrote:
> 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 21:10:44 2025
@@ -695,6 +695,8 @@
 mode_edit(int fd, int page, int edit, int argc, char *argv[])
 {
 	int i;
+	int flag_all;
+	int offset;
 	u_char data[255];
 	u_char *mode_pars;
 	struct mode_header
@@ -804,10 +806,26 @@
 	 */
 	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)
+	 */
+
+	flag_all = ((page & 0x3f) == 0x3f);
+	offset = (flag_all ? 3 : 5);
+
+	mode_pars = (char *)mph + ( flag_all ? 0 : sizeof(*mph) );
+
 	if (!fmt) {
-		for (i = 0; i < mh->mdl; i++) {
+		for (i = 0; i < (mh->mdl - mh->bdl - offset); i++) {
 			printf("%02x%c",mode_pars[i],
 			(((i + 1) % 8) == 0) ? '\n' : ' ');
 		}