Re: [PATCH v2 1/3] hw/i2c: add mctp core

[Corey Minyard]

On Wed, Apr 26, 2023 at 09:11:16AM +0200, Klaus Jensen wrote:
> On Apr 25 10:19, Corey Minyard wrote:
> > On Tue, Apr 25, 2023 at 08:35:38AM +0200, Klaus Jensen wrote:
> > > From: Klaus Jensen <k.jensen@samsung.com>
> > > 
> > > Add an abstract MCTP over I2C endpoint model. This implements MCTP
> > > control message handling as well as handling the actual I2C transport
> > > (packetization).
> > > 
> > > Devices are intended to derive from this and implement the class
> > > methods.
> > > 
> > > Parts of this implementation is inspired by code[1] previously posted by
> > > Jonathan Cameron.
> > 
> > All in all this looks good.  Two comments:
> > 
> > I would like to see the buffer handling consolidated into one function
> > and the length checked, even for (especially for) the outside users of
> > this code, like the nvme code.  Best to avoid future issues with buffer
> > overruns.  This will require reworking the get_message_types function,
> > unfortunately.
> > 
> 
> Right now the implementations (i.e. hw/nvme/nmi-i2c.c) writes directly
> into the mctp core buffer for get_message_bytes(). The contract is that
> it must not write more than the `maxlen` parameter. Is that bad? Would
> it be better that get_message_bytes() returned a pointer to its own
> buffer that hw/mctp can then copy from?

qemu has had several instances of unchecked writing into a buffer
eventually getting it into trouble.  It might be ok in the beginning,
but as things change and code is added, something might come in that is
not ok.

nmi_get_message_types(), for instance, does not check maxlen.

It may be borderline paranoia, but I've seen too many instances where
paranoia was warranted :).

Plus I think it would make the code a little cleaner and easier to
maintain.  If you wanted to change how the buffer worked, trace data put
into the buffer, or something like that, all the code to handle that is
in one place.

> 
> > You have one trace function on a bad receive message check, but lots of
> > other bad receive message checks with no trace.  Just a suggestion, but
> > it might be nice for tracking down issues to trace all the reasons a
> > message is dropped.
> > 
> 
> Sounds reasonable! :)
> 
> Thanks for the review!

Thank you for the submission :).

-corey