Re: [PATCH v3 0/3] hw/{i2c, nvme}: mctp endpoint, nvme management interface model

[Corey Minyard]

On Wed, May 31, 2023 at 01:47:41PM +0200, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> This adds a generic MCTP endpoint model that other devices may derive
> from. I'm not 100% happy with the design of the class methods, but it's
> a start.
> 
> Also included is a very basic implementation of an NVMe-MI device,
> supporting only a small subset of the required commands. Lior (CC'ed) has some
> patches coming up that adds futher support.
> 
> Since this all relies on i2c target mode, this can currently only be
> used with an SoC that includes the Aspeed I2C controller.
> 
> The easiest way to get up and running with this, is to grab my buildroot
> overlay[1]. It includes modified a modified dts as well as a couple of
> required packages.
> 
> QEMU can then be launched along these lines:
> 
>   qemu-system-arm \
>     -nographic \
>     -M ast2600-evb \
>     -kernel output/images/zImage \
>     -initrd output/images/rootfs.cpio \
>     -dtb output/images/aspeed-ast2600-evb-nmi.dtb \
>     -nic user,hostfwd=tcp::2222-:22 \
>     -device nmi-i2c,address=0x3a \
>     -serial mon:stdio
> 
> From within the booted system,
> 
>   mctp addr add 8 dev mctpi2c15
>   mctp link set mctpi2c15 up
>   mctp route add 9 via mctpi2c15
>   mctp neigh add 9 dev mctpi2c15 lladdr 0x3a
>   mi-mctp 1 9 info
> 
> Comments are very welcome!
> 
>   [1]: https://github.com/birkelund/buildroots/tree/main/mctp-i2c
> 
> Changes since v2
> ~~~~~~~~~~~~~~~~
> 
>   - Applied a bunch of feedback from Jonathan:
>     + Moved a lot of internally used structs out of the include headers
>       and into the source files.
>     + Added spec references in various places
>     + Split the patch for i2c_smbus_pec() into its own
>     + Fix a compile error (and bug) in nmi-i2c.c.
> 
>   - From Corey:
>     + Reworked the buffer handling. The deriving devices now returns a
>       pointer to their own buffer that the mctp core copies into.

You didn't do what I asked here, I guess I wasn't clear.  You have:

+static void i2c_mctp_handle_control_set_eid(MCTPI2CEndpoint *mctp, uint8_t eid)
+{
+    mctp->my_eid = eid;
+
+    uint8_t buf[] = {
+        0x0, 0x0, eid, 0x0,
+    };
+
+    memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
+    mctp->len += sizeof(buf);
+}

That style of programming can lead to buffer overruns as code changes,
as you aren't checking the length of the target buffer.  I don't think
there are any issues now, but as people change the code you might end up
with one if someone gets a length wrong.

What I would like is for you to create a function like:

  i2c_mctp_add_bytes(mctp, buf, len)

that checks that len bytes will fit, then does the addition of the
bytes.  You need to adjust this to fit how you are doing things, and you
probably want one that adds just one byte, but hopefully you get the idea.

I'm sorry to be picky, but I've seen and fixed too many buffer overruns
(including one in the qemu i2c code) in situations like this.  Corey's
rule is: Never add anything to a buffer without checking the length.

Everything else looks good.

-corey

>     + Added a couple of extra debugging trace events.
> 
> Changes since v1
> ~~~~~~~~~~~~~~~~
> 
>   - Fix SPDX-License tag for hw/nvme/nmi-i2c.c (Philippe)
>   - Add some asserts to verify buffer indices (by request from Corey).
>   - Drop short packets that could result in underflow (Corey)
>   - Move i2c_smbus_pec() to smbus common code (Corey)
>   - A couple of logic fixes (patch from Jeremy squashed in)
>   - Added a patch to handle messages with dest eid 0 (Matt)
>     Maybe squash this as well.
> 
> Klaus Jensen (3):
>   hw/i2c: add smbus pec utility function
>   hw/i2c: add mctp core
>   hw/nvme: add nvme management interface model
> 
>  MAINTAINERS                   |   7 +
>  hw/arm/Kconfig                |   1 +
>  hw/i2c/Kconfig                |   4 +
>  hw/i2c/mctp.c                 | 398 ++++++++++++++++++++++++++++++++++
>  hw/i2c/meson.build            |   1 +
>  hw/i2c/smbus_master.c         |  28 +++
>  hw/i2c/trace-events           |  13 ++
>  hw/nvme/meson.build           |   1 +
>  hw/nvme/nmi-i2c.c             | 367 +++++++++++++++++++++++++++++++
>  hw/nvme/trace-events          |   6 +
>  include/hw/i2c/mctp.h         | 137 ++++++++++++
>  include/hw/i2c/smbus_master.h |   2 +
>  include/net/mctp.h            |  28 +++
>  13 files changed, 993 insertions(+)
>  create mode 100644 hw/i2c/mctp.c
>  create mode 100644 hw/nvme/nmi-i2c.c
>  create mode 100644 include/hw/i2c/mctp.h
>  create mode 100644 include/net/mctp.h
> 
> -- 
> 2.40.0
> 
>