aspeed/hace: security fixes

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

Posted by Cédric Le Goater 2 weeks, 1 day ago

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

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

Posted by Kane Chen 1 week, 6 days ago

> -----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

Posted by Cédric Le Goater 1 week, 6 days ago

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

Posted by Cédric Le Goater 1 week, 6 days ago

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

Posted by Kane Chen 1 week, 5 days ago

> -----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