Re: [PATCH v3] dt-bindings: misc: add binding for Xilinx AXI-Stream FIFO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing runtime bounds check for tx_fifo_depth causes integer underflow and potential DoS
- [High] Unvalidated hardware read can cause massive busy loop and CPU soft lockup
- [High] Hardware interrupts enabled before IRQ handler registration
--

commit cd9dfa74e4b12151a534638f2622f83e05f40fee
Author: Aditya Chari <adi25charis@gmail.com>

dt-bindings: misc: add binding for Xilinx AXI-Stream FIFO

This patch converts the free-form text binding for the Xilinx AXI-Stream
FIFO IP core into a YAML schema. It also enforces a minimum value of 4
for the xlnx,tx-fifo-depth property to match the driver's transmit bounds
check.

> diff --git a/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s.yaml b/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s.yaml
> new file mode 100644
> index 0000000000000..53bf52524e74d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s.yaml
> @@ -0,0 +1,227 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/xlnx,axi-fifo-mm-s.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - xlnx,axi-fifo-mm-s-4.1
> +      - xlnx,axi-fifo-mm-s-4.2
> +      - xlnx,axi-fifo-mm-s-4.3
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1

[Severity: High]
This is a pre-existing issue, but does the driver enable interrupts before
registering the IRQ handler?

Looking at drivers/staging/axis-fifo/axis-fifo.c:axis_fifo_probe():

    reset_ip_core(fifo);

    irq = platform_get_irq(pdev, 0);
    if (irq < 0)
        return irq;

    rc = devm_request_irq(fifo->dt_device, irq, &axis_fifo_irq, 0,
                          DRIVER_NAME, fifo);

And in reset_ip_core():

    iowrite32(XLLF_INT_TC_MASK | XLLF_INT_RC_MASK | XLLF_INT_RPURE_MASK |
              XLLF_INT_RPORE_MASK | XLLF_INT_RPUE_MASK |
              XLLF_INT_TPOE_MASK | XLLF_INT_TSE_MASK,
              fifo->base_addr + XLLF_IER_OFFSET);

If the hardware immediately asserts an interrupt, could this trigger a spurious
interrupt before devm_request_irq() completes, permanently disabling the IRQ
line?

[ ... ]

> +  xlnx,tx-fifo-depth:
> +    description:
> +      Depth in 32-bit words of the transmit FIFO, as configured in the
> +      IP core. Must be at least 4; the driver subtracts 4 from this
> +      value when bounds-checking transmit writes, and a smaller value
> +      underflows that check.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 4

[Severity: High]
This isn't a bug introduced by this patch, but since the binding enforces a
minimum of 4, what happens if a malformed device tree provides a smaller
value?

In drivers/staging/axis-fifo/axis-fifo.c:axis_fifo_parse_dt():

    ret = of_property_read_u32(node, "xlnx,tx-fifo-depth",
                               &fifo->tx_fifo_depth);
    if (ret)
        return ret;

The value is parsed without enforcing the boundary constraint.  Could this
lead to an integer underflow later in axis_fifo_write()?

    if (!words_to_write || (len % sizeof(u32)) ||
        (words_to_write > (fifo->tx_fifo_depth - 4)))
        return -EINVAL;

If fifo->tx_fifo_depth is less than 4, this subtraction underflows, bypassing
the maximum packet size check.

[ ... ]

> +  xlnx,use-rx-data:
> +    description:
> +      Indicates whether the receive data path is present, as configured
> +      in the IP core. A value of 1 enables the receive path, 0 disables it.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]

[Severity: High]
This is a pre-existing issue, but does the driver handle hardware read errors
safely when processing received data?

In drivers/staging/axis-fifo/axis-fifo.c:axis_fifo_read():

    bytes_available = ioread32(fifo->base_addr + XLLF_RLR_OFFSET);
    words_available = bytes_available / sizeof(u32);
    ...
    if (bytes_available % sizeof(u32)) {
        /* this probably can't happen unless IP
         * registers were previously mishandled
         */
        dev_err(fifo->dt_device, "received a packet that isn't word-aligned\n");
        ret = -EIO;
        goto err_flush_rx;
    }

If the device is in an error state and ioread32() returns 0xFFFFFFFF,
words_available becomes 0x3FFFFFFF.  Would this cause the error recovery loop
to stall the CPU?

    err_flush_rx:
        while (words_available--)
            ioread32(fifo->base_addr + XLLF_RDFD_OFFSET);

Executing over a billion memory-mapped reads sequentially could trigger the
kernel's soft lockup watchdog.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260621094312.53655-1-adi25charis@gmail.com?part=1