diff mbox series

[ovs-dev,v4] lldp: Fix out of bound write in chassisid_to_string.

Message ID 20250608041632.3876844-1-changliang.wu@smartx.com
State Accepted
Commit aea4734299d802ebaabecdf891e24028d0891137
Delegated to: aaron conole
Headers show
Series [ovs-dev,v4] lldp: Fix out of bound write in chassisid_to_string. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/cirrus-robot success cirrus build: passed
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Changliang Wu June 8, 2025, 4:16 a.m. UTC 
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>
---

v2: create new function for hex string with delimiter.
v3: move new function into ds lib.
v4: fix patch format problems.

---
 include/openvswitch/dynamic-string.h |  1 +
 lib/dynamic-string.c                 | 15 ++++++++++++
 lib/ofp-actions.c                    | 16 +++----------
 lib/ofp-packet.c                     | 14 ++---------
 lib/ovs-lldp.c                       | 36 +++++++++-------------------
 5 files changed, 32 insertions(+), 50 deletions(-)

Comments

Simon Horman June 18, 2025, 8:43 a.m. UTC | #1 
On Sun, Jun 08, 2025 at 12:16:32PM +0800, Changliang Wu wrote:
> 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>
> ---
> 
> v2: create new function for hex string with delimiter.
> v3: move new function into ds lib.
> v4: fix patch format problems.

FTR, this has been accepted into main as:

- AUTHORS: Add Changliang Wu.
  https://github.com/openvswitch/ovs/commit/8a1a0ea7c0bd
- lldp: Fix out of bound write in chassisid_to_string.
  https://github.com/openvswitch/ovs/commit/aea4734299d8

Thanks to all.
diff mbox series

Patch

diff --git a/include/openvswitch/dynamic-string.h b/include/openvswitch/dynamic-string.h
index a1d341d48..bd8a5d853 100644
--- a/include/openvswitch/dynamic-string.h
+++ b/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,
diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
index 37b513966..8a7d210c6 100644
--- a/lib/dynamic-string.c
+++ b/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)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 102abbc23..16de931bb 100644
--- a/lib/ofp-actions.c
+++ b/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
diff --git a/lib/ofp-packet.c b/lib/ofp-packet.c
index 9485ddfc9..0236aaaf8 100644
--- a/lib/ofp-packet.c
+++ b/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');
     }
 
diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index 2d13e971e..ab80e09c4 100644
--- a/lib/ovs-lldp.c
+++ b/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;
-            char *system;
+            struct ds id = DS_EMPTY_INITIALIZER;
+            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,
-                                        port->p_chassis->c_id_len, &id);
+                    ds_put_hex_with_delimiter(&id, port->p_chassis->c_id,
+                                   port->p_chassis->c_id_len, ":");
                 }
 
                 descr = port->p_chassis->c_descr;
             }
 
-            chassisid_to_string((uint8_t *) &port->p_element.system_id,
-                sizeof port->p_element.system_id, &system);
+            ds_put_hex_with_delimiter(&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);
-            free(system);
+            ds_destroy(&id);
+            ds_destroy(&system);
         }
     }
 }