[PATCHv2] ata: pata_ep93xx: fix endianness bug in trailing byte transfer

[PATCHv2] ata: pata_ep93xx: fix endianness bug in trailing byte transfer

[Rosen Penev]

ep93xx_pata_data_xfer() handles a trailing odd byte by reading/writing
a full 16-bit word and extracting the first byte.  The current code
uses cpu_to_le16()/le16_to_cpu() wrapping, but then assigns through a
u8 pointer (*pad), truncating the value.

On the READ path, *pad = cpu_to_le16(...) truncates the u16 to u8.
On little-endian this happens to give the right byte, but on big-endian
cpu_to_le16() puts the valid byte in the upper 8 bits and truncation
keeps the lower 8 bits (zeros), losing the data.

On the WRITE path, le16_to_cpu(*pad) zero-extends pad[0] (u8) to u16
(0x00BB) and then on big-endian byte-swaps to 0xBB00.  The hardware then
receives 0x00 as the first byte instead of 0xBB.

The readl()/writel() calls inside ep93xx_pata_read_data() and
ep93xx_pata_write_data() already handle endianness correctly, so the
trailing byte can just use the raw read/write value directly without
the cpu_to_le16/le16_to_cpu wrappers.  Remove the pad[] array and
simplify.

Fixes: 2fff27512600f9ad ("PATA host controller driver for ep93xx")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202607231521.gvhnJpOG-lkp@intel.com/
Assisted-by: Opencode:Big-Pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: add kernel bot fixes tags
 drivers/ata/pata_ep93xx.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
index 339ee5e43e9f..558f9cf95e6e 100644
--- a/drivers/ata/pata_ep93xx.c
+++ b/drivers/ata/pata_ep93xx.c
@@ -499,19 +499,12 @@ static unsigned int ep93xx_pata_data_xfer(struct ata_queued_cmd *qc,
 
 	/* Transfer trailing 1 byte, if any. */
 	if (unlikely(buflen & 0x01)) {
-		unsigned char pad[2] = { };
-
 		buf += buflen - 1;
 
 		if (rw == READ) {
-			*pad = cpu_to_le16(
-				ep93xx_pata_read_data(
-					drv_data, IDECTRL_ADDR_DATA));
-			*buf = pad[0];
+			*buf = ep93xx_pata_read_data(drv_data, IDECTRL_ADDR_DATA);
 		} else {
-			pad[0] = *buf;
-			ep93xx_pata_write_data(drv_data, le16_to_cpu(*pad),
-					  IDECTRL_ADDR_DATA);
+			ep93xx_pata_write_data(drv_data, *buf, IDECTRL_ADDR_DATA);
 		}
 		words++;
 	}
-- 
2.55.0

Re: [PATCHv2] ata: pata_ep93xx: fix endianness bug in trailing byte transfer

[sashiko-bot]

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

Pre-existing issues:
- [High] The `ep93xx_pata_data_xfer` function calculates an incorrect consumed byte count due to the `words` loop counter underflowing, returning either `0xFFFFFFFE` or `0`.
--

--- Patch [1]: [PATCHv2] ata: pata_ep93xx: fix endianness bug in trailing byte transfer ---
commit 6f058310f6ea9721e68fb8c4613fa6b146f2e1f7
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_ep93xx: fix endianness bug in trailing byte transfer

This commit fixes an endianness issue in the trailing odd byte transfer
logic by removing the pad array and directly reading/writing the u8
value without wrappers.

Closes: https://lore.kernel.org/oe-kbuild-all/202607231521.gvhnJpOG-lkp@intel.com/

> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index 42a24dc51d26b..647977ed607c8 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
> @@ -499,19 +499,12 @@ static unsigned int ep93xx_pata_data_xfer(struct ata_queued_cmd *qc,
>  
>  	/* Transfer trailing 1 byte, if any. */
>  	if (unlikely(buflen & 0x01)) {
> -		unsigned char pad[2] = { };
> -
>  		buf += buflen - 1;
>  
>  		if (rw == READ) {
> -			*pad = cpu_to_le16(
> -				ep93xx_pata_read_data(
> -					drv_data, IDECTRL_ADDR_DATA));
> -			*buf = pad[0];
> +			*buf = ep93xx_pata_read_data(drv_data, IDECTRL_ADDR_DATA);
>  		} else {
> -			pad[0] = *buf;
> -			ep93xx_pata_write_data(drv_data, le16_to_cpu(*pad),
> -					  IDECTRL_ADDR_DATA);
> +			ep93xx_pata_write_data(drv_data, *buf, IDECTRL_ADDR_DATA);
>  		}
>  		words++;
>  	}

[Severity: High]
This is a pre-existing issue, but does this function calculate the correct
consumed byte count?

When ep93xx_pata_data_xfer() executes the earlier transfer loop, the words
variable unconditionally decrements until it underflows:

drivers/ata/pata_ep93xx.c:ep93xx_pata_data_xfer() {
    ...
    unsigned int words = buflen >> 1;

    /* Transfer multiple of 2 bytes */
    while (words--)
        if (rw == READ)
            ...
}

After the loop, words underflows to UINT_MAX.

For odd-length transfers, the trailing byte block quoted above executes
words++, which increments UINT_MAX to 0. The function then returns words << 1,
which results in returning 0.

If 0 is returned, could this cause an infinite loop in __atapi_pio_bytes()? A
returned consumed value of 0 means qc->curbytes does not advance, which might
cause the transfer loop to stall indefinitely.

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