On Thu, 7 Nov 2024 15:39:13 +0000
Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Fri, 1 Nov 2024 at 13:43, Jonathan Cameron via <qemu-devel@nongnu.org>
> wrote:
> >
> > In cmd_features_set_feature() the an offset + data size schemed
> > is used to allow for large features. Ensure this does not write
> > beyond the end fo the buffers used to accumulate the full feature
> > attribute set.
> >
> > Reported-by: Esifiel <esif...@gmail.com>
> > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index a40d81219c..078782e8b9 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -1292,6 +1292,11 @@ static CXLRetCode cmd_features_set_feature(const
> > struct cxl_cmd *cmd,
> >
> > ps_set_feature = (void *)payload_in;
> > ps_write_attrs = &ps_set_feature->feat_data;
> > +
> > + if ((uint32_t)hdr->offset + bytes_to_copy >
> > + sizeof(ct3d->patrol_scrub_wr_attrs)) {
> > + return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> > + }
>
> Coverity complains about this code (CID 1564900, 1564901).
> Essentially it does not like that this check permits
> the memcpy for the case where hdr->offset is 2 and
> bytes_to_copy is 0, because memcpy(invalid_dest, src, 0)
> is still UB even though you might logically expect it
> to do nothing.
Huh. Something new I learned today ;)
Anyhow, it makes little sense to have a set feature with zero length payload
so I can check for this before we even know what type of payload this is,
thus catching both cases here.
I'll spin a patch shortly.
thanks
Jonathan
>
> > memcpy((uint8_t *)&ct3d->patrol_scrub_wr_attrs + hdr->offset,
> > ps_write_attrs,
> > bytes_to_copy);
>
> > @@ -1314,6 +1319,11 @@ static CXLRetCode cmd_features_set_feature(const
> > struct cxl_cmd *cmd,
> >
> > ecs_set_feature = (void *)payload_in;
> > ecs_write_attrs = ecs_set_feature->feat_data;
> > +
> > + if ((uint32_t)hdr->offset + bytes_to_copy >
> > + sizeof(ct3d->ecs_wr_attrs)) {
> > + return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> > + }
> > memcpy((uint8_t *)&ct3d->ecs_wr_attrs + hdr->offset,
> > ecs_write_attrs,
> > bytes_to_copy);
>
> Similarly here.
>
> thanks
> -- PMM
