[Linux-parport] [PATCH] IEEE1284 Device ID read fixes

[Marko Kohtala]

I have no working device to test this patch with, so please test it if you 
have one.

My device has this feature broken and it gives all nibbles as value 8, 
forever. This now stops reading the ID sooner and does not overflow the buffer.

I rearranged the code a little to keep it easier to follow.

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/01/09 23:19:37+02:00 kohtala at kohtala.home.org
#   Bugfixes to parport IEEE1284 device ID reading.
#
# drivers/parport/probe.c
#   2005/01/09 23:16:56+02:00 kohtala at kohtala.home.org +74 -61
#   parport_device_id fixes:
#   - Fix a bug in recent change that could leave the device in nibble mode in
#     failed device ID read.
#   - Fix buffer overflow in case of long device ID and buggy device.
#   - Change to match documentation and return the Device ID length in first 
two bytes.
#   - Handle some devices better that do not implement Device ID but negotiate
#     into the mode.
#
diff -Nru a/drivers/parport/probe.c b/drivers/parport/probe.c
--- a/drivers/parport/probe.c	2005-01-10 07:58:48 +02:00
+++ b/drivers/parport/probe.c	2005-01-10 07:58:48 +02:00
@@ -137,8 +137,78 @@
  	kfree(txt);
  }

+/* Read up to count-1 bytes of device id. Terminate buffer with
+ * '\0'. Buffer begins with two Device ID length bytes as give by
+ * device. */
+static ssize_t parport_read_device_id (struct parport *port, char *buffer,
+				       size_t count)
+{
+	size_t idlen;
+	unsigned char length[2];
+	ssize_t retval;
+	ssize_t len = 0;
+
+	/* First two bytes are MSB,LSB of inclusive length. */
+	retval = parport_read (port, length, 2);
+
+	if (retval < 0)
+		return retval;
+	if (retval != 2)
+		return -EIO;
+
+	memcpy(buffer, length, 2);
+	idlen = (length[0] << 8) + length[1];
+	len = 2;
+
+	/* Some devices don't truly implement Device ID, but just
+	   return constant nibble forever. This catches also those
+	   cases. */
+	if (idlen <= 2 || idlen > 0xFFF) {
+		printk (KERN_DEBUG "%s: reported broken Device ID length of %#zX bytes\n",
+			port->name, idlen-2);
+		return -EIO;
+	}
+
+	/* Avoid reading past given ID length just in case some
+	 * devices do not properly implement the end of data in Device
+	 * ID nibble mode. */
+	retval = parport_read (port, buffer+len,
+			       min(count-len-1,idlen-2));
+
+	if (retval < 0)
+		return retval;
+	len += retval;
+
+	/* Some printer manufacturers mistakenly believe that
+	   the length field is supposed to be _exclusive_. */
+	if (len == idlen && len+1 < count && buffer[len-1] != ';') {
+		retval = parport_read (port, buffer + len,
+				       max(2u,count-len-1));
+		if (retval < 0)
+			return retval;
+		if (retval) {
+			printk (KERN_DEBUG
+				"%s: device reported incorrect "
+				"length field (%d, should be %d)\n",
+				port->name, idlen, idlen+2);
+			len += retval;
+		}
+	}
+	else if (retval != idlen-2)
+		printk (KERN_DEBUG "%s: read %zu instead of %zu Device ID bytes\n",
+			port->name, retval,
+			idlen-2);
+
+	/* In addition, there are broken devices out there that don't
+	   even finish off with a semi-colon. We do not need to care
+	   about those this time. */
+
+	buffer[len] = '\0';
+	return len;
+}
+
  /* Get Std 1284 Device ID. */
-ssize_t parport_device_id (int devnum, char *buffer, size_t len)
+ssize_t parport_device_id (int devnum, char *buffer, size_t count)
  {
  	ssize_t retval = -ENXIO;
  	struct pardevice *dev = parport_open (devnum, "Device ID probe",
@@ -155,69 +225,12 @@
  				    IEEE1284_MODE_NIBBLE | IEEE1284_DEVICEID);

  	if (!retval) {
-		int idlen;
-		unsigned char length[2];
-
-		/* First two bytes are MSB,LSB of inclusive length. */
-		retval = parport_read (dev->port, length, 2);
-
-		if (retval != 2) goto end_id;
-
-		idlen = (length[0] << 8) + length[1] - 2;
-		/*
-		 * Check if the caller-allocated buffer is large enough
-		 * otherwise bail out or there will be an at least off by one.
-		 */
-		if (idlen + 1 < len)
-			len = idlen;
-		else {
-			retval = -EINVAL;
-			goto out;
-		}
-		retval = parport_read (dev->port, buffer, len);
-
-		if (retval != len)
-			printk (KERN_DEBUG "%s: only read %Zd of %Zd ID bytes\n",
-				dev->port->name, retval,
-				len);
-
-		/* Some printer manufacturers mistakenly believe that
-                   the length field is supposed to be _exclusive_.
-		   In addition, there are broken devices out there
-                   that don't even finish off with a semi-colon. */
-		if (buffer[len - 1] != ';') {
-			ssize_t diff;
-			diff = parport_read (dev->port, buffer + len, 2);
-			retval += diff;
-
-			if (diff)
-				printk (KERN_DEBUG
-					"%s: device reported incorrect "
-					"length field (%d, should be %Zd)\n",
-					dev->port->name, idlen, retval);
-			else {
-				/* One semi-colon short of a device ID. */
-				buffer[len++] = ';';
-				printk (KERN_DEBUG "%s: faking semi-colon\n",
-					dev->port->name);
-
-				/* If we get here, I don't think we
-                                   need to worry about the possible
-                                   standard violation of having read
-                                   more than we were told to.  The
-                                   device is non-compliant anyhow. */
-			}
-		}
-
-	end_id:
-		buffer[len] = '\0';
+		retval = parport_read_device_id (dev->port, buffer, count);
  		parport_negotiate (dev->port, IEEE1284_MODE_COMPAT);
+		if (retval > 2)
+			parse_data (dev->port, dev->daisy, buffer+2);
  	}

-	if (retval > 2)
-		parse_data (dev->port, dev->daisy, buffer);
-
-out:
  	parport_release (dev);
  	parport_close (dev);
  	return retval;