[PULL 25/27] hw/net/ftgmac100: Improve DMA error handling

Philippe Mathieu-Daudé posted 27 patches 1 week, 3 days ago
Maintainers: Yi Liu <yi.l.liu@intel.com>, Eric Auger <eric.auger@redhat.com>, Zhenzhong Duan <zhenzhong.duan@intel.com>, Richard Henderson <richard.henderson@linaro.org>, Alistair Francis <Alistair.Francis@wdc.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Vijai Kumar K <vijai@behindbytes.com>, Palmer Dabbelt <palmer@dabbelt.com>, "Michael S. Tsirkin" <mst@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Amit Shah <amit@kernel.org>, Jonathan Cameron <jonathan.cameron@huawei.com>, Fan Ni <fan.ni@samsung.com>, Helge Deller <deller@gmx.de>, "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Gerd Hoffmann <kraxel@redhat.com>, Joe Komlodi <komlodi@google.com>, "Cédric Le Goater" <clg@kaod.org>, Jamin Lin <jamin_lin@aspeedtech.com>, Nabih Estefan <nabihestefan@google.com>, Corey Minyard <minyard@acm.org>, Thomas Huth <th.huth+qemu@posteo.eu>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Huacai Chen <chenhuacai@kernel.org>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Peter Maydell <peter.maydell@linaro.org>, Steven Lee <steven_lee@aspeedtech.com>, Troy Lee <leetroy@gmail.com>, Andrew Jeffery <andrew@codeconstruct.com.au>, Joel Stanley <joel@jms.id.au>, Jason Wang <jasowang@redhat.com>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>, Jiri Pirko <jiri@resnulli.us>, Elena Ufimtseva <elena.ufimtseva@oracle.com>, Jagannathan Raman <jag.raman@oracle.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Chao Liu <chao.liu.zevorn@gmail.com>, Fam Zheng <fam@euphon.net>, Cornelia Huck <cohuck@redhat.com>, Eric Farman <farman@linux.ibm.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Tony Krowiak <akrowiak@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Jason Herne <jjherne@linux.ibm.com>, Alex Williamson <alex@shazbot.org>, Stefano Garzarella <sgarzare@redhat.com>, Magnus Kulke <magnuskulke@linux.microsoft.com>, Wei Liu <wei.liu@kernel.org>, "Dr. David Alan Gilbert" <dave@treblig.org>, Marcelo Tosatti <mtosatti@redhat.com>
[PULL 25/27] hw/net/ftgmac100: Improve DMA error handling
Posted by Philippe Mathieu-Daudé 1 week, 3 days ago
From: Cédric Le Goater <clg@redhat.com>

Currently, DMA memory operation errors in the ftgmac100 model are not
all tested and this can lead to a guest-triggerable denial of service
as described in https://gitlab.com/qemu-project/qemu/-/work_items/3335.

To fix this, check the return value of ftgmac100_write_bd() in the TX
path and exit the TX loop on error to prevent further processing. In
the event of a DMA error, also set FTGMAC100_INT_AHB_ERR interrupt
flag as appropriate.

The FTGMAC100_INT_AHB_ERR interrupt status bit only applies to the
AST2400 SoC; on newer Aspeed SoCs, it is a reserved bit.
Nevertheless, since it is supported by the Linux driver and it should
be safe to use in the QEMU implementation across all SoCs.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335
Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-ID: <20260322215732.387383-3-clg@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/ftgmac100.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index d29f7dcd171..2f05bba11d0 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -624,7 +624,10 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t tx_ring,
         bd.des0 &= ~FTGMAC100_TXDES0_TXDMA_OWN;
 
         /* Write back the modified descriptor.  */
-        ftgmac100_write_bd(&bd, addr);
+        if (ftgmac100_write_bd(&bd, addr)) {
+            s->isr |= FTGMAC100_INT_AHB_ERR;
+            break;
+        }
         /* Advance to the next descriptor.  */
         if (bd.des0 & s->txdes0_edotr) {
             addr = tx_ring;
@@ -1134,7 +1137,10 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
             bd.des0 |= flags | FTGMAC100_RXDES0_LRS;
             s->isr |= FTGMAC100_INT_RPKT_BUF;
         }
-        ftgmac100_write_bd(&bd, addr);
+        if (ftgmac100_write_bd(&bd, addr)) {
+            s->isr |= FTGMAC100_INT_AHB_ERR;
+            break;
+        }
         if (bd.des0 & s->rxdes0_edorr) {
             addr = s->rx_ring;
         } else {
-- 
2.53.0


Re: [PULL 25/27] hw/net/ftgmac100: Improve DMA error handling
Posted by Cédric Le Goater 1 week, 3 days ago
Hello,

On 3/23/26 17:52, Philippe Mathieu-Daudé wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> Currently, DMA memory operation errors in the ftgmac100 model are not
> all tested and this can lead to a guest-triggerable denial of service
> as described in https://gitlab.com/qemu-project/qemu/-/work_items/3335.
> 
> To fix this, check the return value of ftgmac100_write_bd() in the TX
> path and exit the TX loop on error to prevent further processing. In
> the event of a DMA error, also set FTGMAC100_INT_AHB_ERR interrupt
> flag as appropriate.
> 
> The FTGMAC100_INT_AHB_ERR interrupt status bit only applies to the
> AST2400 SoC; on newer Aspeed SoCs, it is a reserved bit.
> Nevertheless, since it is supported by the Linux driver and it should
> be safe to use in the QEMU implementation across all SoCs.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335


Philippe,

You can certainly apply the fixes, but I don't see any email from you
about it. However, I indicated that I was applying it to aspeed-next
as part of this series:

   https://lore.kernel.org/qemu-devel/20260323125545.577653-1-clg@redhat.com/

There is reason for it. The SMC patch is required to fix issue 3335.
The patch should say so, I agree. My bad.

I see that some VFIO patches are also included. I appreciate your help,
but please contact the maintainers before, I believe I am responsive
enough to requests ? I will rework the aspeed PR I had prepared
and update the VFIO tree.


Thanks,

C.


> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-ID: <20260322215732.387383-3-clg@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/net/ftgmac100.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index d29f7dcd171..2f05bba11d0 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -624,7 +624,10 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t tx_ring,
>           bd.des0 &= ~FTGMAC100_TXDES0_TXDMA_OWN;
>   
>           /* Write back the modified descriptor.  */
> -        ftgmac100_write_bd(&bd, addr);
> +        if (ftgmac100_write_bd(&bd, addr)) {
> +            s->isr |= FTGMAC100_INT_AHB_ERR;
> +            break;
> +        }
>           /* Advance to the next descriptor.  */
>           if (bd.des0 & s->txdes0_edotr) {
>               addr = tx_ring;
> @@ -1134,7 +1137,10 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>               bd.des0 |= flags | FTGMAC100_RXDES0_LRS;
>               s->isr |= FTGMAC100_INT_RPKT_BUF;
>           }
> -        ftgmac100_write_bd(&bd, addr);
> +        if (ftgmac100_write_bd(&bd, addr)) {
> +            s->isr |= FTGMAC100_INT_AHB_ERR;
> +            break;
> +        }
>           if (bd.des0 & s->rxdes0_edorr) {
>               addr = s->rx_ring;
>           } else {


Re: [PULL 25/27] hw/net/ftgmac100: Improve DMA error handling
Posted by Philippe Mathieu-Daudé 1 week, 2 days ago
Hi Cédric,

On 24/3/26 09:03, Cédric Le Goater wrote:
> Hello,
> 
> On 3/23/26 17:52, Philippe Mathieu-Daudé wrote:
>> From: Cédric Le Goater <clg@redhat.com>
>>
>> Currently, DMA memory operation errors in the ftgmac100 model are not
>> all tested and this can lead to a guest-triggerable denial of service
>> as described in https://gitlab.com/qemu-project/qemu/-/work_items/3335.
>>
>> To fix this, check the return value of ftgmac100_write_bd() in the TX
>> path and exit the TX loop on error to prevent further processing. In
>> the event of a DMA error, also set FTGMAC100_INT_AHB_ERR interrupt
>> flag as appropriate.
>>
>> The FTGMAC100_INT_AHB_ERR interrupt status bit only applies to the
>> AST2400 SoC; on newer Aspeed SoCs, it is a reserved bit.
>> Nevertheless, since it is supported by the Linux driver and it should
>> be safe to use in the QEMU implementation across all SoCs.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335
> 
> 
> Philippe,
> 
> You can certainly apply the fixes, but I don't see any email from you
> about it. However, I indicated that I was applying it to aspeed-next
> as part of this series:
> 
>    https://lore.kernel.org/qemu-devel/20260323125545.577653-1- 
> clg@redhat.com/
> 
> There is reason for it. The SMC patch is required to fix issue 3335.
> The patch should say so, I agree. My bad.
> 
> I see that some VFIO patches are also included. I appreciate your help,
> but please contact the maintainers before, I believe I am responsive
> enough to requests ? I will rework the aspeed PR I had prepared
> and update the VFIO tree.

I understood your aspeed-next branch was for the first queue to apply
after 11.0, not for fixes before, and this one felt worthwhile fix.
Indeed I should have replied to the patch with that justification
and you'd have cleared any doubts. I apologize for having stepped over
your maintenance area. This shouldn't reproduce.

Regards,

Phil.

> 
> 
> Thanks,
> 
> C.


Re: [PULL 25/27] hw/net/ftgmac100: Improve DMA error handling
Posted by Cédric Le Goater 1 week, 2 days ago
On 3/24/26 20:21, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
> 
> On 24/3/26 09:03, Cédric Le Goater wrote:
>> Hello,
>>
>> On 3/23/26 17:52, Philippe Mathieu-Daudé wrote:
>>> From: Cédric Le Goater <clg@redhat.com>
>>>
>>> Currently, DMA memory operation errors in the ftgmac100 model are not
>>> all tested and this can lead to a guest-triggerable denial of service
>>> as described in https://gitlab.com/qemu-project/qemu/-/work_items/3335.
>>>
>>> To fix this, check the return value of ftgmac100_write_bd() in the TX
>>> path and exit the TX loop on error to prevent further processing. In
>>> the event of a DMA error, also set FTGMAC100_INT_AHB_ERR interrupt
>>> flag as appropriate.
>>>
>>> The FTGMAC100_INT_AHB_ERR interrupt status bit only applies to the
>>> AST2400 SoC; on newer Aspeed SoCs, it is a reserved bit.
>>> Nevertheless, since it is supported by the Linux driver and it should
>>> be safe to use in the QEMU implementation across all SoCs.
>>>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3335
>>
>>
>> Philippe,
>>
>> You can certainly apply the fixes, but I don't see any email from you
>> about it. However, I indicated that I was applying it to aspeed-next
>> as part of this series:
>>
>>    https://lore.kernel.org/qemu-devel/20260323125545.577653-1- clg@redhat.com/
>>
>> There is reason for it. The SMC patch is required to fix issue 3335.
>> The patch should say so, I agree. My bad.
>>
>> I see that some VFIO patches are also included. I appreciate your help,
>> but please contact the maintainers before, I believe I am responsive
>> enough to requests ? I will rework the aspeed PR I had prepared
>> and update the VFIO tree.
> 
> I understood your aspeed-next branch was for the first queue to apply
> after 11.0, not for fixes before, and this one felt worthwhile fix.
> Indeed I should have replied to the patch with that justification
> and you'd have cleared any doubts. I apologize for having stepped over
> your maintenance area. This shouldn't reproduce.

I am sure it will :) No harm done. Just some slight frustration
on my side.

Cheers,

C.