[PATCH qemu 00/10] hw/cxl: Mailbox input parser hardening against invalid input.

[PATCH qemu 00/10] hw/cxl: Mailbox input parser hardening against invalid input.

[Jonathan Cameron via]

The CXL device mailbox has some variable sized input commands. The payload
length for each must be established using command especific structures.

If user space is either buggy or malicious, it may use size fields to
indicate fields beyond the end of the payload sent.  Some checks on this
were missing and Esifiel picked up on this.  I've tagged all these fixes
with Esifiel's Reported-by as either they were in the report or are similar
issues in other commands.

These can mostly be easily tested by using the raw mailbox commands option
in Linux and injecting broken commands from user space.

A typical command needs to first check that there is enough data to get to
the command specific sizing fields, then check the reported size is less
than or equal to the available payload.

Note that I think it very unlikely anyone is currently using CXL emulation
with a VM that they do not trust, but that may happen in future so good to
fix these paths now.

Jonathan Cameron (10):
  hw/cxl: Check size of input data to dynamic capacity mailbox commands
  hw/cxl: Check input includes at least the header in
    cmd_features_set_feature()
  hw/cxl: Check input length is large enough in
    cmd_events_clear_records()
  hw/cxl: Check enough data in cmd_firmware_update_transfer()
  hw/cxl: Check the length of data requested fits in get_log()
  hw/cxl: Avoid accesses beyond the end of cel_log.
  hw/cxl: Ensuring enough data to read parameters in
    cmd_tunnel_management_cmd()
  hw/cxl: Check that writes do not go beyond end of target attributes
  hw/cxl: Ensure there is enough data for the header in
    cmd_ccls_set_lsa()
  hw/cxl: Ensure there is enough data to read the input header in
    cmd_get_physical_port_state()

 hw/cxl/cxl-mailbox-utils.c | 73 ++++++++++++++++++++++++++++++++------
 1 file changed, 62 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH qemu 01/10] hw/cxl: Check size of input data to dynamic capacity mailbox commands

[Jonathan Cameron via]

cxl_cmd_dcd_release_dyn_cap() and cmd_dcd_add_dyn_cap_rsp() are missing
input message size checks.  These must be done in the individual
commands when the command has a variable length input payload.

A buggy or malicious guest might send undersized messages via the mailbox.
As that size is used to take a copy of the mailbox content, each command
must check there is sufficient data. In this case the first check is that
there is enough data to read how many extents there are, and the second
that there is enough for those elements to be accessed.

Reported-by: Esifiel <esifiel@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 97cb8bbcec..17924410dd 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -2465,11 +2465,20 @@ static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
     uint64_t dpa, len;
     CXLRetCode ret;
 
+    if (len_in < sizeof(*in)) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
     if (in->num_entries_updated == 0) {
         cxl_extent_group_list_delete_front(&ct3d->dc.extents_pending);
         return CXL_MBOX_SUCCESS;
     }
 
+    if (len_in <
+        sizeof(*in) + sizeof(*in->updated_entries) * in->num_entries_updated) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
     /* Adding extents causes exceeding device's extent tracking ability. */
     if (in->num_entries_updated + ct3d->dc.total_extent_count >
         CXL_NUM_EXTENTS_SUPPORTED) {
@@ -2624,10 +2633,19 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
     uint32_t updated_list_size;
     CXLRetCode ret;
 
+    if (len_in < sizeof(*in)) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
     if (in->num_entries_updated == 0) {
         return CXL_MBOX_INVALID_INPUT;
     }
 
+    if (len_in <
+        sizeof(*in) + sizeof(*in->updated_entries) * in->num_entries_updated) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
     ret = cxl_detect_malformed_extent_list(ct3d, in);
     if (ret != CXL_MBOX_SUCCESS) {
         return ret;
-- 
2.43.0

Re: [PATCH qemu 01/10] hw/cxl: Check size of input data to dynamic capacity mailbox commands

[Fan Ni]

On Fri, Nov 01, 2024 at 01:39:08PM +0000, Jonathan Cameron wrote:
> cxl_cmd_dcd_release_dyn_cap() and cmd_dcd_add_dyn_cap_rsp() are missing
> input message size checks.  These must be done in the individual
> commands when the command has a variable length input payload.
> 
> A buggy or malicious guest might send undersized messages via the mailbox.
> As that size is used to take a copy of the mailbox content, each command
> must check there is sufficient data. In this case the first check is that
> there is enough data to read how many extents there are, and the second
> that there is enough for those elements to be accessed.
> 
> Reported-by: Esifiel <esifiel@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Fan Ni <fan.ni@samsung.com>

> ---
>  hw/cxl/cxl-mailbox-utils.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 97cb8bbcec..17924410dd 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -2465,11 +2465,20 @@ static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const struct cxl_cmd *cmd,
>      uint64_t dpa, len;
>      CXLRetCode ret;
>  
> +    if (len_in < sizeof(*in)) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
>      if (in->num_entries_updated == 0) {
>          cxl_extent_group_list_delete_front(&ct3d->dc.extents_pending);
>          return CXL_MBOX_SUCCESS;
>      }
>  
> +    if (len_in <
> +        sizeof(*in) + sizeof(*in->updated_entries) * in->num_entries_updated) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
>      /* Adding extents causes exceeding device's extent tracking ability. */
>      if (in->num_entries_updated + ct3d->dc.total_extent_count >
>          CXL_NUM_EXTENTS_SUPPORTED) {
> @@ -2624,10 +2633,19 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
>      uint32_t updated_list_size;
>      CXLRetCode ret;
>  
> +    if (len_in < sizeof(*in)) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
>      if (in->num_entries_updated == 0) {
>          return CXL_MBOX_INVALID_INPUT;
>      }
>  
> +    if (len_in <
> +        sizeof(*in) + sizeof(*in->updated_entries) * in->num_entries_updated) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
>      ret = cxl_detect_malformed_extent_list(ct3d, in);
>      if (ret != CXL_MBOX_SUCCESS) {
>          return ret;
> -- 
> 2.43.0
> 

-- 
Fan Ni

[PATCH qemu 02/10] hw/cxl: Check input includes at least the header in cmd_features_set_feature()

[Jonathan Cameron via]

A buggy guest might write an insufficiently large message.
Check the header is present. Whilst zero data after the header is very
odd it will just result in failure to copy any data.

Reported-by: Esifiel <esifiel@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 17924410dd..e63140aefe 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1238,6 +1238,9 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
     CXLType3Dev *ct3d;
     uint16_t count;
 
+    if (len_in < sizeof(*hdr)) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
 
     if (!object_dynamic_cast(OBJECT(cci->d), TYPE_CXL_TYPE3)) {
         return CXL_MBOX_UNSUPPORTED;
-- 
2.43.0

Re: [PATCH qemu 02/10] hw/cxl: Check input includes at least the header in cmd_features_set_feature()

[Fan Ni]

On Fri, Nov 01, 2024 at 01:39:09PM +0000, Jonathan Cameron wrote:
> A buggy guest might write an insufficiently large message.
> Check the header is present. Whilst zero data after the header is very
> odd it will just result in failure to copy any data.
> 
> Reported-by: Esifiel <esifiel@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

Reviewed-by: Fan Ni <fan.ni@samsung.com>

>  hw/cxl/cxl-mailbox-utils.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 17924410dd..e63140aefe 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1238,6 +1238,9 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>      CXLType3Dev *ct3d;
>      uint16_t count;
>  
> +    if (len_in < sizeof(*hdr)) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
>  
>      if (!object_dynamic_cast(OBJECT(cci->d), TYPE_CXL_TYPE3)) {
>          return CXL_MBOX_UNSUPPORTED;
> -- 
> 2.43.0
> 

-- 
Fan Ni

[PATCH qemu 03/10] hw/cxl: Check input length is large enough in cmd_events_clear_records()

[Jonathan Cameron via]

Buggy software might write a message that is too short for
either the header, or the header + the event data that is specified
in the header.  This may result in accesses beyond the range of the
message allocated as a duplicate of the incoming message buffer.

Reported-by: Esifiel <esifiel@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index e63140aefe..3cb499a24f 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -266,6 +266,12 @@ static CXLRetCode cmd_events_clear_records(const struct cxl_cmd *cmd,
     CXLClearEventPayload *pl;
 
     pl = (CXLClearEventPayload *)payload_in;
+
+    if (len_in < sizeof(*pl) ||
+        len_in < sizeof(*pl) + sizeof(*pl->handle) * pl->nr_recs) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
     *len_out = 0;
     return cxl_event_clear_records(cxlds, pl);
 }
-- 
2.43.0

Re: [PATCH qemu 03/10] hw/cxl: Check input length is large enough in cmd_events_clear_records()

[Fan Ni]

On Fri, Nov 01, 2024 at 01:39:10PM +0000, Jonathan Cameron wrote:
> Buggy software might write a message that is too short for
> either the header, or the header + the event data that is specified
> in the header.  This may result in accesses beyond the range of the
> message allocated as a duplicate of the incoming message buffer.
> 
> Reported-by: Esifiel <esifiel@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

Reviewed-by: Fan Ni <fan.ni@samsung.com>

>  hw/cxl/cxl-mailbox-utils.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index e63140aefe..3cb499a24f 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -266,6 +266,12 @@ static CXLRetCode cmd_events_clear_records(const struct cxl_cmd *cmd,
>      CXLClearEventPayload *pl;
>  
>      pl = (CXLClearEventPayload *)payload_in;
> +
> +    if (len_in < sizeof(*pl) ||
> +        len_in < sizeof(*pl) + sizeof(*pl->handle) * pl->nr_recs) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
>      *len_out = 0;
>      return cxl_event_clear_records(cxlds, pl);
>  }
> -- 
> 2.43.0
> 

-- 
Fan Ni

[PATCH qemu 04/10] hw/cxl: Check enough data in cmd_firmware_update_transfer()

[Jonathan Cameron via]

Buggy guest can write a message that advertises more data that
is provided. As QEMU internally duplicates the reported message
size, this may result in an out of bounds access.
Add sanity checks on the size to avoid this.

Reported-by: Esifiel <esifiel@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 3cb499a24f..27fadc4fa8 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -705,6 +705,10 @@ static CXLRetCode cmd_firmware_update_transfer(const struct cxl_cmd *cmd,
     } QEMU_PACKED *fw_transfer = (void *)payload_in;
     size_t offset, length;
 
+    if (len < sizeof(*fw_transfer)) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
     if (fw_transfer->action == CXL_FW_XFER_ACTION_ABORT) {
         /*
          * At this point there aren't any on-going transfers
-- 
2.43.0

Re: [PATCH qemu 04/10] hw/cxl: Check enough data in cmd_firmware_update_transfer()

[Fan Ni]

On Fri, Nov 01, 2024 at 01:39:11PM +0000, Jonathan Cameron wrote:
> Buggy guest can write a message that advertises more data that
> is provided. As QEMU internally duplicates the reported message
> size, this may result in an out of bounds access.
> Add sanity checks on the size to avoid this.
> 
> Reported-by: Esifiel <esifiel@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

Reviewed-by: Fan Ni <fan.ni@samsung.com>

>  hw/cxl/cxl-mailbox-utils.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 3cb499a24f..27fadc4fa8 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -705,6 +705,10 @@ static CXLRetCode cmd_firmware_update_transfer(const struct cxl_cmd *cmd,
>      } QEMU_PACKED *fw_transfer = (void *)payload_in;
>      size_t offset, length;
>  
> +    if (len < sizeof(*fw_transfer)) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
>      if (fw_transfer->action == CXL_FW_XFER_ACTION_ABORT) {
>          /*
>           * At this point there aren't any on-going transfers
> -- 
> 2.43.0
> 

-- 
Fan Ni

[PATCH qemu 05/10] hw/cxl: Check the length of data requested fits in get_log()

[Jonathan Cameron via]

Checking offset + length is of no relevance when verifying the CEL
data will fit in the mailbox payload. Only the length is is relevant.

Note that this removes a potential overflow.

Reported-by: Esifiel <esifiel@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 27fadc4fa8..2aa7ffed84 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -947,7 +947,7 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd,
      * the only possible failure would be if the mailbox itself isn't big
      * enough.
      */
-    if (get_log->offset + get_log->length > cci->payload_max) {
+    if (get_log->length > cci->payload_max) {
         return CXL_MBOX_INVALID_INPUT;
     }
 
-- 
2.43.0

Re: [PATCH qemu 05/10] hw/cxl: Check the length of data requested fits in get_log()

[Fan Ni]

On Fri, Nov 01, 2024 at 01:39:12PM +0000, Jonathan Cameron wrote:
> Checking offset + length is of no relevance when verifying the CEL
> data will fit in the mailbox payload. Only the length is is relevant.
s/is is/is/
> 
> Note that this removes a potential overflow.
> 
> Reported-by: Esifiel <esifiel@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  hw/cxl/cxl-mailbox-utils.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 27fadc4fa8..2aa7ffed84 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -947,7 +947,7 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd,
>       * the only possible failure would be if the mailbox itself isn't big
>       * enough.
>       */
> -    if (get_log->offset + get_log->length > cci->payload_max) {
> +    if (get_log->length > cci->payload_max) {

If offset is beyond the size of cel_log, will it be a problem?

There is a comment just above saying "
 * The CEL buffer is large enough to fit all commands in the emulation, so
 * the only possible failure would be if the mailbox itself isn't big
 * enough.
 "

Not sure how it avoids the case when the offset is too large.

Fan

>          return CXL_MBOX_INVALID_INPUT;
>      }
>  
> -- 
> 2.43.0
> 

-- 
Fan Ni

[PATCH qemu 06/10] hw/cxl: Avoid accesses beyond the end of cel_log.

[Jonathan Cameron via]

Add a check that the requested offset + length does not go beyond the end
of the cel_log.

Whilst the cci->cel_log is large enough to include all possible CEL
entries, the guest might still ask for entries beyond the end of it.
Move the comment to this new check rather than before the check on the
type of log requested.

Reported-by: Esifiel <esifiel@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 2aa7ffed84..5e571955b6 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -937,24 +937,28 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd,
 
     get_log = (void *)payload_in;
 
+    if (get_log->length > cci->payload_max) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    if (!qemu_uuid_is_equal(&get_log->uuid, &cel_uuid)) {
+        return CXL_MBOX_INVALID_LOG;
+    }
+
     /*
      * CXL r3.1 Section 8.2.9.5.2: Get Log (Opcode 0401h)
      *   The device shall return Invalid Input if the Offset or Length
      *   fields attempt to access beyond the size of the log as reported by Get
-     *   Supported Logs.
+     *   Supported Log.
      *
-     * The CEL buffer is large enough to fit all commands in the emulation, so
-     * the only possible failure would be if the mailbox itself isn't big
-     * enough.
+     * Only valid for there to be one entry per opcode, but the length + offset
+     * may still be greater than that if the inputs are not valid and so access
+     * beyond the end of cci->cel_log.
      */
-    if (get_log->length > cci->payload_max) {
+    if ((uint64_t)get_log->offset + get_log->length >= sizeof(cci->cel_log)) {
         return CXL_MBOX_INVALID_INPUT;
     }
 
-    if (!qemu_uuid_is_equal(&get_log->uuid, &cel_uuid)) {
-        return CXL_MBOX_INVALID_LOG;
-    }
-
     /* Store off everything to local variables so we can wipe out the payload */
     *len_out = get_log->length;
 
-- 
2.43.0

Re: [PATCH qemu 06/10] hw/cxl: Avoid accesses beyond the end of cel_log.

[Fan Ni]

On Fri, Nov 01, 2024 at 01:39:13PM +0000, Jonathan Cameron wrote:
> Add a check that the requested offset + length does not go beyond the end
> of the cel_log.
> 
> Whilst the cci->cel_log is large enough to include all possible CEL
> entries, the guest might still ask for entries beyond the end of it.
> Move the comment to this new check rather than before the check on the
> type of log requested.
> 
> Reported-by: Esifiel <esifiel@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  hw/cxl/cxl-mailbox-utils.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 2aa7ffed84..5e571955b6 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -937,24 +937,28 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd,
>  
>      get_log = (void *)payload_in;
>  
> +    if (get_log->length > cci->payload_max) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    if (!qemu_uuid_is_equal(&get_log->uuid, &cel_uuid)) {
> +        return CXL_MBOX_INVALID_LOG;
> +    }
> +
>      /*
>       * CXL r3.1 Section 8.2.9.5.2: Get Log (Opcode 0401h)
>       *   The device shall return Invalid Input if the Offset or Length
>       *   fields attempt to access beyond the size of the log as reported by Get
> -     *   Supported Logs.
> +     *   Supported Log.
>       *
> -     * The CEL buffer is large enough to fit all commands in the emulation, so
> -     * the only possible failure would be if the mailbox itself isn't big
> -     * enough.
> +     * Only valid for there to be one entry per opcode, but the length + offset
> +     * may still be greater than that if the inputs are not valid and so access
> +     * beyond the end of cci->cel_log.
>       */
> -    if (get_log->length > cci->payload_max) {
> +    if ((uint64_t)get_log->offset + get_log->length >= sizeof(cci->cel_log)) {
>          return CXL_MBOX_INVALID_INPUT;

Oh. This patch actually addresses my concern in the previous patch.
Maybe we can combine the changes in this patch and the previous one
together. Other than that 

Reviewed-by: Fan Ni <fan.ni@samsung.com>

>      }
>  
> -    if (!qemu_uuid_is_equal(&get_log->uuid, &cel_uuid)) {
> -        return CXL_MBOX_INVALID_LOG;
> -    }
> -
>      /* Store off everything to local variables so we can wipe out the payload */
>      *len_out = get_log->length;
>  
> -- 
> 2.43.0
> 

-- 
Fan Ni

[PATCH qemu 07/10] hw/cxl: Ensuring enough data to read parameters in cmd_tunnel_management_cmd()

[Jonathan Cameron via]

If len_in is less than the minimum spec allowed value, then return
CXL_MBOX_INVALID_PAYLOAD_LENGTH

Reported-by: Esifiel <esifiel@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 5e571955b6..a40d81219c 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -151,6 +151,9 @@ static CXLRetCode cmd_tunnel_management_cmd(const struct cxl_cmd *cmd,
     in = (void *)payload_in;
     out = (void *)payload_out;
 
+    if (len_in < sizeof(*in)) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
     /* Enough room for minimum sized message - no payload */
     if (in->size < sizeof(in->ccimessage)) {
         return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
-- 
2.43.0

Re: [PATCH qemu 07/10] hw/cxl: Ensuring enough data to read parameters in cmd_tunnel_management_cmd()

[Fan Ni]

On Fri, Nov 01, 2024 at 01:39:14PM +0000, Jonathan Cameron wrote:
> If len_in is less than the minimum spec allowed value, then return
> CXL_MBOX_INVALID_PAYLOAD_LENGTH
> 
> Reported-by: Esifiel <esifiel@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  hw/cxl/cxl-mailbox-utils.c | 3 +++
>  1 file changed, 3 insertions(+)
> 

Reviewed-by: Fan Ni <fan.ni@samsung.com>

> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 5e571955b6..a40d81219c 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -151,6 +151,9 @@ static CXLRetCode cmd_tunnel_management_cmd(const struct cxl_cmd *cmd,
>      in = (void *)payload_in;
>      out = (void *)payload_out;
>  
> +    if (len_in < sizeof(*in)) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
>      /* Enough room for minimum sized message - no payload */
>      if (in->size < sizeof(in->ccimessage)) {
>          return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> -- 
> 2.43.0
> 

-- 
Fan Ni

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

[Jonathan Cameron via]

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

[Fan Ni]

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

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

[Peter Maydell]

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

[Jonathan Cameron via]

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

[PATCH qemu 09/10] hw/cxl: Ensure there is enough data for the header in cmd_ccls_set_lsa()

[Jonathan Cameron via]

The properties of the requested set command cannot be established if
len_in is less than the size of the header.

Reported-by: Esifiel <esifiel@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 078782e8b9..f4a436e172 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1503,8 +1503,8 @@ static CXLRetCode cmd_ccls_set_lsa(const struct cxl_cmd *cmd,
     const size_t hdr_len = offsetof(struct set_lsa_pl, data);
 
     *len_out = 0;
-    if (!len_in) {
-        return CXL_MBOX_SUCCESS;
+    if (len_in < hdr_len) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
     }
 
     if (set_lsa_payload->offset + len_in > cvc->get_lsa_size(ct3d) + hdr_len) {
-- 
2.43.0

Re: [PATCH qemu 09/10] hw/cxl: Ensure there is enough data for the header in cmd_ccls_set_lsa()

[Fan Ni]

On Fri, Nov 01, 2024 at 01:39:16PM +0000, Jonathan Cameron wrote:
> The properties of the requested set command cannot be established if
> len_in is less than the size of the header.
> 
> Reported-by: Esifiel <esifiel@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

Reviewed-by: Fan Ni <fan.ni@samsung.com>

>  hw/cxl/cxl-mailbox-utils.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 078782e8b9..f4a436e172 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1503,8 +1503,8 @@ static CXLRetCode cmd_ccls_set_lsa(const struct cxl_cmd *cmd,
>      const size_t hdr_len = offsetof(struct set_lsa_pl, data);
>  
>      *len_out = 0;
> -    if (!len_in) {
> -        return CXL_MBOX_SUCCESS;
> +    if (len_in < hdr_len) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
>      }
>  
>      if (set_lsa_payload->offset + len_in > cvc->get_lsa_size(ct3d) + hdr_len) {
> -- 
> 2.43.0
> 

-- 
Fan Ni

[PATCH qemu 10/10] hw/cxl: Ensure there is enough data to read the input header in cmd_get_physical_port_state()

[Jonathan Cameron via]

If len_in is smaller than the header length then the accessing the
number of ports will result in an out of bounds access.
Add a check to avoid this.

Reported-by: Esifiel <esifiel@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index f4a436e172..2d4d62c454 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -530,6 +530,9 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
     in = (struct cxl_fmapi_get_phys_port_state_req_pl *)payload_in;
     out = (struct cxl_fmapi_get_phys_port_state_resp_pl *)payload_out;
 
+    if (len_in < sizeof(*in)) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
     /* Check if what was requested can fit */
     if (sizeof(*out) + sizeof(*out->ports) * in->num_ports > cci->payload_max) {
         return CXL_MBOX_INVALID_INPUT;
-- 
2.43.0

Re: [PATCH qemu 10/10] hw/cxl: Ensure there is enough data to read the input header in cmd_get_physical_port_state()

[Fan Ni]

On Fri, Nov 01, 2024 at 01:39:17PM +0000, Jonathan Cameron wrote:
> If len_in is smaller than the header length then the accessing the
> number of ports will result in an out of bounds access.
> Add a check to avoid this.
> 
> Reported-by: Esifiel <esifiel@gmail.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

Reviewed-by: Fan Ni <fan.ni@samsung.com>

>  hw/cxl/cxl-mailbox-utils.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index f4a436e172..2d4d62c454 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -530,6 +530,9 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>      in = (struct cxl_fmapi_get_phys_port_state_req_pl *)payload_in;
>      out = (struct cxl_fmapi_get_phys_port_state_resp_pl *)payload_out;
>  
> +    if (len_in < sizeof(*in)) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
>      /* Check if what was requested can fit */
>      if (sizeof(*out) + sizeof(*out->ports) * in->num_ports > cci->payload_max) {
>          return CXL_MBOX_INVALID_INPUT;
> -- 
> 2.43.0
> 

-- 
Fan Ni

Re: [PATCH qemu 00/10] hw/cxl: Mailbox input parser hardening against invalid input.

[Jonathan Cameron via]

On Fri, 1 Nov 2024 13:39:07 +0000
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> The CXL device mailbox has some variable sized input commands. The payload
> length for each must be established using command especific structures.
> 
> If user space is either buggy or malicious, it may use size fields to
> indicate fields beyond the end of the payload sent.  Some checks on this
> were missing and Esifiel picked up on this.  I've tagged all these fixes
> with Esifiel's Reported-by as either they were in the report or are similar
> issues in other commands.
> 
> These can mostly be easily tested by using the raw mailbox commands option
> in Linux and injecting broken commands from user space.
> 
> A typical command needs to first check that there is enough data to get to
> the command specific sizing fields, then check the reported size is less
> than or equal to the available payload.
> 
> Note that I think it very unlikely anyone is currently using CXL emulation
> with a VM that they do not trust, but that may happen in future so good to
> fix these paths now.

Sorry, forgot to list dependencies.

Based-on: [PATCH 0/7] hw/cxl: Round up of fixes.
Based-on: [PATCH qemu 0/2] hw/cxl: Misc fixes

Based-on: Message-id: 20241014121902.2146424-1-Jonathan.Cameron@huawei.com
Based-on; message-id: 20241101132005.26633-1-Jonathan.Cameron@huawei.com


> 
> Jonathan Cameron (10):
>   hw/cxl: Check size of input data to dynamic capacity mailbox commands
>   hw/cxl: Check input includes at least the header in
>     cmd_features_set_feature()
>   hw/cxl: Check input length is large enough in
>     cmd_events_clear_records()
>   hw/cxl: Check enough data in cmd_firmware_update_transfer()
>   hw/cxl: Check the length of data requested fits in get_log()
>   hw/cxl: Avoid accesses beyond the end of cel_log.
>   hw/cxl: Ensuring enough data to read parameters in
>     cmd_tunnel_management_cmd()
>   hw/cxl: Check that writes do not go beyond end of target attributes
>   hw/cxl: Ensure there is enough data for the header in
>     cmd_ccls_set_lsa()
>   hw/cxl: Ensure there is enough data to read the input header in
>     cmd_get_physical_port_state()
> 
>  hw/cxl/cxl-mailbox-utils.c | 73 ++++++++++++++++++++++++++++++++------
>  1 file changed, 62 insertions(+), 11 deletions(-)
>