From: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Date: Tue, 7 May 2019 13:58:29 +0000 (+0200)
Subject: scsi_serial: replace some crazy strncpy() calls by strnlen()
X-Git-Tag: v243-rc1~474^2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=099c77fd5ff835614dea8dc11c57f6d44f77d9ee;p=thirdparty%2Fsystemd.git

scsi_serial: replace some crazy strncpy() calls by strnlen()

gcc was warning about strncpy() leaving an unterminated string.
In this case, it was correct.

The code was doing strncpy()+strncat()+strlen() essentially to determine
if the strings have expected length. If the length was correct, a buffer
overread was performed (or at least some garbage bytes were used from the
uninitialized part of the buffer). Let's do the length check first and then
only copy stuff if everything agrees.

For some reason the function was called "prepend", when it obviously does
an "append".
---

diff --git a/src/udev/scsi_id/scsi_serial.c b/src/udev/scsi_id/scsi_serial.c
index 0abc8ca2128..e1940e7d38f 100644
--- a/src/udev/scsi_id/scsi_serial.c
+++ b/src/udev/scsi_id/scsi_serial.c
@@ -396,27 +396,21 @@ static int do_scsi_page0_inquiry(struct scsi_id_device *dev_scsi, int fd,
         return 0;
 }
 
-/*
- * The caller checks that serial is long enough to include the vendor +
- * model.
- */
-static int prepend_vendor_model(struct scsi_id_device *dev_scsi, char *serial) {
-        int ind;
-
-        strncpy(serial, dev_scsi->vendor, VENDOR_LENGTH);
-        strncat(serial, dev_scsi->model, MODEL_LENGTH);
-        ind = strlen(serial);
+static int append_vendor_model(
+                const struct scsi_id_device *dev_scsi,
+                char buf[static VENDOR_LENGTH + MODEL_LENGTH]) {
 
-        /*
-         * This is not a complete check, since we are using strncat/cpy
-         * above, ind will never be too large.
-         */
-        if (ind != (VENDOR_LENGTH + MODEL_LENGTH))
+        if (strnlen(dev_scsi->vendor, VENDOR_LENGTH) != VENDOR_LENGTH)
+                return log_debug_errno(SYNTHETIC_ERRNO(EINVAL),
+                                       "%s: bad vendor string \"%s\"",
+                                       dev_scsi->kernel, dev_scsi->vendor);
+        if (strnlen(dev_scsi->model, MODEL_LENGTH) != MODEL_LENGTH)
                 return log_debug_errno(SYNTHETIC_ERRNO(EINVAL),
-                                       "%s: expected length %d, got length %d",
-                                       dev_scsi->kernel,
-                                       (VENDOR_LENGTH + MODEL_LENGTH), ind);
-        return ind;
+                                       "%s: bad model string \"%s\"",
+                                       dev_scsi->kernel, dev_scsi->model);
+        memcpy(buf, dev_scsi->vendor, VENDOR_LENGTH);
+        memcpy(buf + VENDOR_LENGTH, dev_scsi->model, MODEL_LENGTH);
+        return VENDOR_LENGTH + MODEL_LENGTH;
 }
 
 /*
@@ -497,7 +491,7 @@ static int check_fill_0x83_id(struct scsi_id_device *dev_scsi,
          * included in the identifier.
          */
         if (id_search->id_type == SCSI_ID_VENDOR_SPECIFIC)
-                if (prepend_vendor_model(dev_scsi, serial + 1) < 0)
+                if (append_vendor_model(dev_scsi, serial + 1) < 0)
                         return 1;
 
         i = 4; /* offset to the start of the identifier */
@@ -731,7 +725,7 @@ static int do_scsi_page80_inquiry(struct scsi_id_device *dev_scsi, int fd,
         len = buf[3];
         if (serial) {
                 serial[0] = 'S';
-                ser_ind = prepend_vendor_model(dev_scsi, serial + 1);
+                ser_ind = append_vendor_model(dev_scsi, serial + 1);
                 if (ser_ind < 0)
                         return 1;
                 ser_ind++; /* for the leading 'S' */