hw/cxl: Mailbox input parser hardening against invalid input.

[PATCH qemu 08/10] hw/cxl: Check that writes do not go beyond end of target attributes

Posted by Jonathan Cameron via 1 year, 3 months ago

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;
+        }
         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

Re: [PATCH qemu 08/10] hw/cxl: Check that writes do not go beyond end of target attributes

Posted by Peter Maydell 1 year, 3 months ago

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

Re: [PATCH qemu 08/10] hw/cxl: Check that writes do not go beyond end of target attributes

Posted by Jonathan Cameron via 1 year, 3 months ago

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

Re: [PATCH qemu 08/10] hw/cxl: Check that writes do not go beyond end of target attributes

Posted by Fan Ni 1 year, 3 months ago

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
> 

-- 
Fan Ni