On Mon, 4 Apr 2022 12:19:07 -0700
Tong Zhang <ztong0...@gmail.com> wrote:
> > On Apr 4, 2022, at 8:14 AM, Jonathan Cameron via <qemu-devel@nongnu.org>
> > wrote:
> >
> > From: Ben Widawsky <ben.widaw...@intel.com>
> >
> > A device's volatile and persistent memory are known Host Defined Memory
> > (HDM) regions. The mechanism by which the device is programmed to claim
> > the addresses associated with those regions is through dedicated logic
> > known as the HDM decoder. In order to allow the OS to properly program
> > the HDMs, the HDM decoders must be modeled.
> >
> > There are two ways the HDM decoders can be implemented, the legacy
> > mechanism is through the PCIe DVSEC programming from CXL 1.1 (8.1.3.8),
> > and MMIO is found in 8.2.5.12 of the spec. For now, 8.1.3.8 is not
> > implemented.
> >
> > Much of CXL device logic is implemented in cxl-utils. The HDM decoder
> > however is implemented directly by the device implementation.
> > Whilst the implementation currently does no validity checks on the
> > encoder set up, future work will add sanity checking specific to
> > the type of cxl component.
> >
> > Signed-off-by: Ben Widawsky <ben.widaw...@intel.com>
> > Co-developed-by: Jonathan Cameron <jonathan.came...@huawei.com>
> > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
> > Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
> > ---
> > hw/mem/cxl_type3.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 55 insertions(+)
> >
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 329a6ea2a9..5c93fbbd9b 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
...
> > +
> > +static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
> > + unsigned size)
> > +{
> > + CXLComponentState *cxl_cstate = opaque;
> > + ComponentRegisters *cregs = &cxl_cstate->crb;
> > + CXLType3Dev *ct3d = container_of(cxl_cstate, CXLType3Dev, cxl_cstate);
> > + uint32_t *cache_mem = cregs->cache_mem_registers;
> > + bool should_commit = false;
> > + int which_hdm = -1;
> > +
> > + assert(size == 4);
> > + g_assert(offset <= CXL2_COMPONENT_CM_REGION_SIZE);
> > +
>
> Looks like this will allow offset == CXL2_COMPONENT_CM_REGION_SIZE to pass
> the check, and cause a buffer overrun.
> Shouldn’t this be g_assert(offset< CXL2_COMPONENT_CM_REGION_SIZE)?
Good point. Silly mistake. I'll fix for v10.
> We also need to make sure (offset + 4<= CXL2_COMPONENT_CM_REGION_SIZE).
> Or maybe we just need offset +4 <= CXL2_COMPONENT_CM_REGION_SIZE here, if
> offset < CXL2_COMPONENT_CM_REGION_SIZE is already checked somewhere else.
I think we are fine without these two because in cxl-components-utils we
restrict the accesses to aligned only.
Jonathan
>
> > + switch (offset) {
> > + case A_CXL_HDM_DECODER0_CTRL:
> > + should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> > + which_hdm = 0;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + stl_le_p((uint8_t *)cache_mem + offset, value);
> > + if (should_commit) {
> > + hdm_decoder_commit(ct3d, which_hdm);
> > + }
> > +}
