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