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

[PATCH] 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.

Assisted-by: Opencode:Big-Pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 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 3f61712af5bb..a0dfdacf54e5 100644
--- a/drivers/ata/pata_ep93xx.c
+++ b/drivers/ata/pata_ep93xx.c
@@ -495,19 +495,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.54.0

Re: [PATCH] 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 destroys the `words` loop counter, returning an incorrect byte count that breaks ATAPI PIO transfers.
--

commit c92ab9f0ecc44ddedc25d0e57302962565e67769
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_ep93xx: fix endianness bug in trailing byte transfer

This commit fixes an endianness bug in ep93xx_pata_data_xfer() where a trailing
odd byte was being truncated and incorrectly byte-swapped on big-endian
systems. It removes the cpu_to_le16/le16_to_cpu wrappers and uses the raw
read/write values directly.

> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
> @@ -495,19 +495,12 @@ static unsigned int ep93xx_pata_data_xfer(struct ata_queued_cmd *qc,
>  
>  	/* Transfer trailing 1 byte, if any. */
>  	if (unlikely(buflen & 0x01)) {

[ ... ]

>  		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 return an incorrect byte
count that breaks ATAPI PIO transfers?

Earlier in ep93xx_pata_data_xfer(), there is a loop that destroys the words
counter:

        unsigned int words = buflen >> 1;
        while (words--)
            /* transfer 2 bytes */

Because of the post-decrement, the words counter underflows to UINT_MAX when
the loop terminates. If there is an odd trailing byte, words++ wraps it back
to 0, and the function returns 0 since the end of the function is:

        return words << 1;

If there is no trailing byte, the function returns UINT_MAX << 1, which is
0xFFFFFFFE.

Returning 0 for an odd-length transfer causes callers like
__atapi_pio_bytes() to not decrement their remaining bytes counter while
still advancing the scatter-gather list, resulting in unexpected or too
much trailing data errors.

Returning 0xFFFFFFFE causes __atapi_pio_bytes() to subtract that huge amount
in a single iteration, instantly dropping the remaining transfer count to zero
even if the DRQ block spanned multiple scatter-gather entries.

Could this be fixed by saving the original buffer length and returning that
instead of relying on the mutated words counter?

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