| Message ID | 20241101133917.27634-9-Jonathan.Cameron@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/cxl: Mailbox input parser hardening against invalid input. | expand |
On Fri, Nov 01, 2024 at 01:39:15PM +0000, Jonathan Cameron wrote: > In cmd_features_set_feature() the an offset + data size schemed Not 100% sure if I get it right, but s/the an/the/. > is used to allow for large features. Ensure this does not write > beyond the end fo the buffers used to accumulate the full feature s/fo/of/ > attribute set. Other than that, Reviewed-by: Fan Ni <fan.ni@samsung.com> > > Reported-by: Esifiel <esifiel@gmail.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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; > + } > 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); > -- > 2.43.0 >
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 <esifiel@gmail.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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. > 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
On Thu, 7 Nov 2024 15:39:13 +0000 Peter Maydell <peter.maydell@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 <esifiel@gmail.com> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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
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; + } 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);
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 <esifiel@gmail.com> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- hw/cxl/cxl-mailbox-utils.c | 10 ++++++++++ 1 file changed, 10 insertions(+)