[PATCH 0/2] aspeed/hace: security fixes

[PATCH 0/2] aspeed/hace: security fixes

[Cédric Le Goater]

Hello,

A couple of issues in the Aspeed HACE model were reported on the
qemu-security list. Here are fixes.

Thanks,

C.

Cédric Le Goater (2):
  aspeed/hace: Fix out-of-bounds read in has_padding()
  aspeed/hace: Prevent total_req_len overflow

 hw/misc/aspeed_hace.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

-- 
2.54.0

[PATCH 1/2] aspeed/hace: Fix out-of-bounds read in has_padding()

[Cédric Le Goater]

The has_padding() function reads the last 8 bytes of a DMA buffer
without validating req_len. req_len is guest-controlled (via
R_HASH_SRC_LEN register or scatter-gather entries) and values less
than 8 cause integer underflow. This can result in an out-of-bounds
read of QEMU process memory.

Add a check to ensure req_len >= 8 before accessing the buffer.

Reported-by: Katherine Leaver <katherine.j.leaver@gmail.com>
Cc: qemu-stable@nongnu.org
Fixes: 5cd7d8564a8b ("aspeed/hace: Support AST2600 HACE")
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/misc/aspeed_hace.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
index 23e8030cd966..ef698b3ecb72 100644
--- a/hw/misc/aspeed_hace.c
+++ b/hw/misc/aspeed_hace.c
@@ -154,6 +154,14 @@ static bool has_padding(AspeedHACEState *s, struct iovec *iov,
                         hwaddr req_len, uint32_t *total_msg_len,
                         uint32_t *pad_offset)
 {
+    /* Need at least 8 bytes to read the total message length field */
+    if (req_len < 8) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid request length=0x%" HWADDR_PRIx "\n",
+                      __func__, req_len);
+        return false;
+    }
+
     *total_msg_len = (uint32_t)(ldq_be_p(iov->iov_base + req_len - 8) / 8);
     /*
      * SG_LIST_LEN_LAST asserted in the request length doesn't mean it is the
-- 
2.54.0

[PATCH 2/2] aspeed/hace: Prevent total_req_len overflow

[Cédric Le Goater]

In accumulate mode, total_req_len is incremented with plen (hwaddr)
for each hash request. Repeated additions can overflow total_req_len
(uint32_t) and potentially bypass validation checks in has_padding().

Add a helper function to detect overflow before incrementing
total_req_len and reject the request if overflow would occur.

Reported-by: Katherine Leaver <katherine.j.leaver@gmail.com>
Cc: qemu-stable@nongnu.org
Fixes: 5cd7d8564a8b ("aspeed/hace: Support AST2600 HACE")
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/misc/aspeed_hace.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
index ef698b3ecb72..0504d61cbf0a 100644
--- a/hw/misc/aspeed_hace.c
+++ b/hw/misc/aspeed_hace.c
@@ -205,6 +205,19 @@ static uint64_t hash_get_source_addr(AspeedHACEState *s)
     return src_addr;
 }
 
+static bool hash_accumulate_len(AspeedHACEState *s, hwaddr plen)
+{
+    if (plen > UINT32_MAX - s->total_req_len) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: total_req_len overflow, current=0x%x, adding=0x%"
+                      HWADDR_PRIx "\n", __func__, s->total_req_len, plen);
+        return false;
+    }
+
+    s->total_req_len += plen;
+    return true;
+}
+
 static int hash_prepare_direct_iov(AspeedHACEState *s, struct iovec *iov,
                                    bool acc_mode, bool *acc_final_request)
 {
@@ -232,7 +245,9 @@ static int hash_prepare_direct_iov(AspeedHACEState *s, struct iovec *iov,
     iov_idx = 1;
 
     if (acc_mode) {
-        s->total_req_len += plen;
+        if (!hash_accumulate_len(s, plen)) {
+            return -1;
+        }
 
         if (has_padding(s, &iov[0], plen, &total_msg_len,
                         &pad_offset)) {
@@ -299,7 +314,9 @@ static int hash_prepare_sg_iov(AspeedHACEState *s, struct iovec *iov,
 
         iov[iov_idx].iov_base = haddr;
         if (acc_mode) {
-            s->total_req_len += plen;
+            if (!hash_accumulate_len(s, plen)) {
+                return -1;
+            }
 
             if (has_padding(s, &iov[iov_idx], plen, &total_msg_len,
                             &pad_offset)) {
-- 
2.54.0

RE: [PATCH 0/2] aspeed/hace: security fixes

[Kane Chen]

> -----Original Message-----
> From: Cédric Le Goater <clg@redhat.com>
> Sent: Tuesday, May 5, 2026 5:34 AM
> To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: Peter Maydell <peter.maydell@linaro.org>; Steven Lee
> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Jamin Lin
> <jamin_lin@aspeedtech.com>; Kane Chen <kane_chen@aspeedtech.com>; Cé
> dric Le Goater <clg@redhat.com>
> Subject: [PATCH 0/2] aspeed/hace: security fixes
> 
> Hello,
> 
> A couple of issues in the Aspeed HACE model were reported on the
> qemu-security list. Here are fixes.
> 
> Thanks,
> 
> C.
> 
> Cédric Le Goater (2):
>   aspeed/hace: Fix out-of-bounds read in has_padding()
>   aspeed/hace: Prevent total_req_len overflow
> 
>  hw/misc/aspeed_hace.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> --
> 2.54.0

Hi Cédric,

Thank you for fixing the defects. These changes look good. However,
I have one question regarding another potential issue.

In the do_hash_operation function, after calling hash_prepare_sg_iov
and hash_prepare_direct_iov, the function may return early if iov_idx
is -1. In this case, the code flow will not reach hash_execute_acc_mode
or hash_execute_non_acc_mode, which means the mapped address may not be
unmapped properly.

I think we may also need to unmap the address when this error condition
occurs. Do you think this should be included in this patch series, or
would a separate patch be preferred?

If a separate patch is preferred, then for this patch series:
Reviewed-by: Kane Chen kane_chen@aspeedtech.com

Best Regards,
Kane

Re: [PATCH 0/2] aspeed/hace: security fixes

[Cédric Le Goater]

On 5/6/26 11:53, Kane Chen wrote:
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Tuesday, May 5, 2026 5:34 AM
>> To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
>> Cc: Peter Maydell <peter.maydell@linaro.org>; Steven Lee
>> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Jamin Lin
>> <jamin_lin@aspeedtech.com>; Kane Chen <kane_chen@aspeedtech.com>; Cé
>> dric Le Goater <clg@redhat.com>
>> Subject: [PATCH 0/2] aspeed/hace: security fixes
>>
>> Hello,
>>
>> A couple of issues in the Aspeed HACE model were reported on the
>> qemu-security list. Here are fixes.
>>
>> Thanks,
>>
>> C.
>>
>> Cédric Le Goater (2):
>>    aspeed/hace: Fix out-of-bounds read in has_padding()
>>    aspeed/hace: Prevent total_req_len overflow
>>
>>   hw/misc/aspeed_hace.c | 29 +++++++++++++++++++++++++++--
>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> --
>> 2.54.0
> 
> Hi Cédric,
> 
> Thank you for fixing the defects. These changes look good. However,
> I have one question regarding another potential issue.
> 
> In the do_hash_operation function, after calling hash_prepare_sg_iov
> and hash_prepare_direct_iov, the function may return early if iov_idx
> is -1. In this case, the code flow will not reach hash_execute_acc_mode
> or hash_execute_non_acc_mode, which means the mapped address may not be
> unmapped properly.
> 
> I think we may also need to unmap the address when this error condition
> occurs. Do you think this should be included in this patch series, or
> would a separate patch be preferred?
> 
> If a separate patch is preferred, 

yes. Do you have time for it ?

> then for this patch series:
> Reviewed-by: Kane Chen kane_chen@aspeedtech.com

Thanks,

C.

Re: [PATCH 0/2] aspeed/hace: security fixes

[Cédric Le Goater]

On 5/6/26 11:53, Kane Chen wrote:
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Tuesday, May 5, 2026 5:34 AM
>> To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
>> Cc: Peter Maydell <peter.maydell@linaro.org>; Steven Lee
>> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Jamin Lin
>> <jamin_lin@aspeedtech.com>; Kane Chen <kane_chen@aspeedtech.com>; Cé
>> dric Le Goater <clg@redhat.com>
>> Subject: [PATCH 0/2] aspeed/hace: security fixes
>>
>> Hello,
>>
>> A couple of issues in the Aspeed HACE model were reported on the
>> qemu-security list. Here are fixes.
>>
>> Thanks,
>>
>> C.
>>
>> Cédric Le Goater (2):
>>    aspeed/hace: Fix out-of-bounds read in has_padding()
>>    aspeed/hace: Prevent total_req_len overflow
>>
>>   hw/misc/aspeed_hace.c | 29 +++++++++++++++++++++++++++--
>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> --
>> 2.54.0
> 
> Hi Cédric,
> 
> Thank you for fixing the defects. These changes look good. However,
> I have one question regarding another potential issue.
> 
> In the do_hash_operation function, after calling hash_prepare_sg_iov
> and hash_prepare_direct_iov, the function may return early if iov_idx
> is -1. In this case, the code flow will not reach hash_execute_acc_mode
> or hash_execute_non_acc_mode, which means the mapped address may not be
> unmapped properly.
> 
> I think we may also need to unmap the address when this error condition
> occurs. Do you think this should be included in this patch series, or
> would a separate patch be preferred?
> 
> If a separate patch is preferred, then for this patch series:
> Reviewed-by: Kane Chen kane_chen@aspeedtech.com

Should be :

Reviewed-by: Kane Chen <kane_chen@aspeedtech.com>

Thanks,

C.

RE: [PATCH 0/2] aspeed/hace: security fixes

[Kane Chen]

> -----Original Message-----
> From: Cédric Le Goater <clg@redhat.com>
> Sent: Wednesday, May 6, 2026 8:28 PM
> To: Kane Chen <kane_chen@aspeedtech.com>; qemu-devel@nongnu.org;
> qemu-arm@nongnu.org
> Cc: Peter Maydell <peter.maydell@linaro.org>; Steven Lee
> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Jamin Lin
> <jamin_lin@aspeedtech.com>
> Subject: Re: [PATCH 0/2] aspeed/hace: security fixes
> 
> On 5/6/26 11:53, Kane Chen wrote:
> >> -----Original Message-----
> >> From: Cédric Le Goater <clg@redhat.com>
> >> Sent: Tuesday, May 5, 2026 5:34 AM
> >> To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
> >> Cc: Peter Maydell <peter.maydell@linaro.org>; Steven Lee
> >> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Jamin Lin
> >> <jamin_lin@aspeedtech.com>; Kane Chen <kane_chen@aspeedtech.com>;
> Cé
> >> dric Le Goater <clg@redhat.com>
> >> Subject: [PATCH 0/2] aspeed/hace: security fixes
> >>
> >> Hello,
> >>
> >> A couple of issues in the Aspeed HACE model were reported on the
> >> qemu-security list. Here are fixes.
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >> Cédric Le Goater (2):
> >>    aspeed/hace: Fix out-of-bounds read in has_padding()
> >>    aspeed/hace: Prevent total_req_len overflow
> >>
> >>   hw/misc/aspeed_hace.c | 29 +++++++++++++++++++++++++++--
> >>   1 file changed, 27 insertions(+), 2 deletions(-)
> >>
> >> --
> >> 2.54.0
> >
> > Hi Cédric,
> >
> > Thank you for fixing the defects. These changes look good. However, I
> > have one question regarding another potential issue.
> >
> > In the do_hash_operation function, after calling hash_prepare_sg_iov
> > and hash_prepare_direct_iov, the function may return early if iov_idx
> > is -1. In this case, the code flow will not reach
> > hash_execute_acc_mode or hash_execute_non_acc_mode, which means the
> > mapped address may not be unmapped properly.
> >
> > I think we may also need to unmap the address when this error
> > condition occurs. Do you think this should be included in this patch
> > series, or would a separate patch be preferred?
> >
> > If a separate patch is preferred,
> 
> yes. Do you have time for it ?
> 
> > then for this patch series:
> > Reviewed-by: Kane Chen kane_chen@aspeedtech.com
> 
> Thanks,
> 
> C.

Hi Cédric,

Sure. I will submit a patch for fixing the potential issue.

Best Regards,
Kane

Re: [PATCH 2/2] aspeed/hace: Prevent total_req_len overflow

[Michael Tokarev]

On 05.05.2026 00:34, Cédric Le Goater wrote:
> In accumulate mode, total_req_len is incremented with plen (hwaddr)
> for each hash request. Repeated additions can overflow total_req_len
> (uint32_t) and potentially bypass validation checks in has_padding().
> 
> Add a helper function to detect overflow before incrementing
> total_req_len and reject the request if overflow would occur.
> 
> Reported-by: Katherine Leaver <katherine.j.leaver@gmail.com>
> Cc: qemu-stable@nongnu.org
> Fixes: 5cd7d8564a8b ("aspeed/hace: Support AST2600 HACE")
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

This change does not apply to 10.0.x qemu-stable series, because,
at least, it lacks v10.0.0-1171-g7328c48b57c9 "hw/misc/aspeed_hace:
Extract direct mode hash buffer setup into helper function".
This code in 10.0.x is slightly different, namely, there's no
error return from the function in question.  Please take a look
at 
https://gitlab.com/mjt0k/qemu/-/commits/4a82112f40351e94f1b3af938782298abda1c810
which is my attempt to fix this code for 10.0.x (not even compile-
tested yet).

Thanks,

/mjt