lldp: Fix out of bound write in chassisid_to_string.

snprintf will automatically write \0 at the end of the string,
and the last one byte will be out of bound.

create a new function ds_put_hex_with_delimiter,
instead of chassisid_to string and format_hex_arg.

Found in sanitize test.

Signed-off-by: Changliang Wu <changliang.wu@smartx.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>

include/openvswitch/dynamic-string.h

@@ -62,6 +62,7 @@ void ds_put_format_valist(struct ds *, const char *, va_list)
 void ds_put_printable(struct ds *, const char *, size_t);
 void ds_put_uuid(struct ds *, const struct uuid *);
 void ds_put_hex(struct ds *ds, const void *buf, size_t size);
+void ds_put_hex_with_delimiter(struct ds *, const void *, size_t, char *);
 void ds_put_hex_dump(struct ds *ds, const void *buf_, size_t size,
                      uintptr_t ofs, bool ascii);
 void ds_put_sparse_hex_dump(struct ds *ds, const void *buf_, size_t size,

lib/dynamic-string.c

@@ -403,6 +403,21 @@ ds_put_hex(struct ds *ds, const void *buf_, size_t size)
     }
 }
 
+void
+ds_put_hex_with_delimiter(struct ds *ds, const void *buf_, size_t size,
+                          char *delimiter)
+{
+    const uint8_t *buf = buf_;
+    size_t i;
+
+    for (i = 0; i < size; i++) {
+        if (i && delimiter) {
+            ds_put_format(ds, "%s", delimiter);
+        }
+        ds_put_format(ds, "%02" PRIx8, buf[i]);
+    }
+}
+
 static void
 ds_put_hex_dump__(struct ds *ds, const void *buf_, size_t size,
                   uintptr_t ofs, bool ascii, bool skip_zero_lines)

lib/ofp-actions.c

@@ -1023,17 +1023,6 @@ parse_CONTROLLER(char *arg, const struct ofpact_parse_params *pp)
     return NULL;
 }
 
-static void
-format_hex_arg(struct ds *s, const uint8_t *data, size_t len)
-{
-    for (size_t i = 0; i < len; i++) {
-        if (i) {
-            ds_put_char(s, '.');
-        }
-        ds_put_format(s, "%02"PRIx8, data[i]);
-    }
-}
-
 static void
 format_CONTROLLER(const struct ofpact_controller *a,
                   const struct ofpact_format_params *fp)
@@ -1063,7 +1052,8 @@ format_CONTROLLER(const struct ofpact_controller *a,
         }
         if (a->userdata_len) {
             ds_put_format(fp->s, "%suserdata=%s", colors.param, colors.end);
-            format_hex_arg(fp->s, a->userdata, a->userdata_len);
+            ds_put_hex_with_delimiter(fp->s, a->userdata, a->userdata_len,
+                                      ".");
             ds_put_char(fp->s, ',');
         }
         if (a->pause) {
@@ -5960,7 +5950,7 @@ format_NOTE(const struct ofpact_note *a,
             const struct ofpact_format_params *fp)
 {
     ds_put_format(fp->s, "%snote:%s", colors.param, colors.end);
-    format_hex_arg(fp->s, a->data, a->length);
+    ds_put_hex_with_delimiter(fp->s, a->data, a->length, ".");
 }
 
 static enum ofperr

lib/ofp-packet.c

@@ -916,17 +916,6 @@ ofputil_decode_packet_in_private(const struct ofp_header *oh, bool loose,
     return error;
 }
 
-static void
-format_hex_arg(struct ds *s, const uint8_t *data, size_t len)
-{
-    for (size_t i = 0; i < len; i++) {
-        if (i) {
-            ds_put_char(s, '.');
-        }
-        ds_put_format(s, "%02"PRIx8, data[i]);
-    }
-}
-
 void
 ofputil_packet_in_private_format(struct ds *s,
                                  const struct ofputil_packet_in_private *pin,
@@ -973,7 +962,8 @@ ofputil_packet_in_private_format(struct ds *s,
 
     if (public->userdata_len) {
         ds_put_cstr(s, " userdata=");
-        format_hex_arg(s, pin->base.userdata, pin->base.userdata_len);
+        ds_put_hex_with_delimiter(s, pin->base.userdata,
+                                  pin->base.userdata_len, ".");
         ds_put_char(s, '\n');
     }
 

lib/ovs-lldp.c

@@ -101,21 +101,6 @@ static struct hmap *const all_mappings OVS_GUARDED_BY(mutex) = &all_mappings__;
 
 static struct lldp_aa_element_system_id system_id_null;
 
-/* Convert an LLDP chassis ID to a string.
- */
-static void
-chassisid_to_string(uint8_t *array, size_t len, char **str)
-{
-    unsigned int i;
-
-    *str = xmalloc(len * 3);
-
-    for (i = 0; i < len; i++) {
-        snprintf(&(*str)[i * 3], 4, "%02x:", array[i]);
-    }
-    (*str)[(i * 3) - 1] = '\0';
-}
-
 /* Find an Auto Attach mapping keyed by I-SID.
  */
 static struct aa_mapping_internal *
@@ -204,30 +189,31 @@ aa_print_element_status_port(struct ds *ds, struct lldpd_hardware *hw)
                    sizeof port->p_element.system_id)) {
             const char *none_str = "<None>";
             const char *descr = NULL;
-            char *id = NULL;
+            struct ds id = DS_EMPTY_INITIALIZER;
-            char *system;
+            struct ds system = DS_EMPTY_INITIALIZER;
 
             if (port->p_chassis) {
                 if (port->p_chassis->c_id_len > 0) {
-                    chassisid_to_string(port->p_chassis->c_id,
+                    ds_put_hex_with_delimiter(&id, port->p_chassis->c_id,
-                                        port->p_chassis->c_id_len, &id);
+                                   port->p_chassis->c_id_len, ":");
                 }
 
                 descr = port->p_chassis->c_descr;
             }
 
-            chassisid_to_string((uint8_t *) &port->p_element.system_id,
+            ds_put_hex_with_delimiter(&system,
-                sizeof port->p_element.system_id, &system);
+                                      (uint8_t *) &port->p_element.system_id,
+                                      sizeof port->p_element.system_id, ":");
 
             ds_put_format(ds, "  Auto Attach Primary Server Id: %s\n",
-                          id ? id : none_str);
+                          id.length ? ds_cstr_ro(&id) : none_str);
             ds_put_format(ds, "  Auto Attach Primary Server Descr: %s\n",
                           descr ? descr : none_str);
             ds_put_format(ds, "  Auto Attach Primary Server System Id: %s\n",
-                          system);
+                          id.length ? ds_cstr_ro(&system) : none_str);
 
-            free(id);
+            ds_destroy(&id);
-            free(system);
+            ds_destroy(&system);
         }
     }
 }