linux-kernel - Re: [PATCH v2] scsi: 3w-9xxx: Fix endianness issues found by sparse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a2p7dWhhCqAYF_Zos-X-zBK+id-xO5hPu2fRTbNyPo9Xg@mail.gmail.com>
Date:   Fri, 31 Jul 2020 09:29:56 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Adam Radford <aradford@...il.com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        linux-scsi <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] scsi: 3w-9xxx: Fix endianness issues found by sparse

On Fri, Jul 31, 2020 at 12:07 AM Samuel Holland <samuel@...lland.org> wrote:
>
> The main issue observed was at the call to scsi_set_resid, where the
> byteswapped parameter would eventually trigger the alignment check at
> drivers/scsi/sd.c:2009. At that point, the kernel would continuously
> complain about an "Unaligned partial completion", and no further I/O
> could occur.
>
> This gets the controller working on big endian powerpc64.
>
> Signed-off-by: Samuel Holland <samuel@...lland.org>
> ---
>
> Changes since v1:
>  - Include changes to use __le?? types in command structures
>  - Use an object literal for the intermediate "schedulertime" value
>  - Use local "error" variable to avoid repeated byte swapping
>  - Create a local "length" variable to avoid very long lines
>  - Move byte swapping to TW_REQ_LUN_IN/TW_LUN_OUT to avoid long lines
>

Looks much better, thanks for the update. I see one more issue here
>  /* Command Packet */
>  typedef struct TW_Command {
> -       unsigned char opcode__sgloffset;
> -       unsigned char size;
> -       unsigned char request_id;
> -       unsigned char unit__hostid;
> +       u8      opcode__sgloffset;
> +       u8      size;
> +       u8      request_id;
> +       u8      unit__hostid;
>         /* Second DWORD */
> -       unsigned char status;
> -       unsigned char flags;
> +       u8      status;
> +       u8      flags;
>         union {
> -               unsigned short block_count;
> -               unsigned short parameter_count;
> +               __le16  block_count;
> +               __le16  parameter_count;
>         } byte6_offset;
>         union {
>                 struct {
> -                       u32 lba;
> -                       TW_SG_Entry sgl[TW_ESCALADE_MAX_SGL_LENGTH];
> -                       dma_addr_t padding;
> +                       __le32          lba;
> +                       TW_SG_Entry     sgl[TW_ESCALADE_MAX_SGL_LENGTH];
> +                       dma_addr_t      padding;


The use of dma_addr_t here seems odd, since this is neither endian-safe nor
fixed-length. I see you replaced the dma_addr_t in TW_SG_Entry with
a variable-length fixed-endian word. I guess there is a chance that this is
correct, but it is really confusing. On top of that, it seems that there is
implied padding in the structure when built with a 64-bit dma_addr_t
on most architectures but not on x86-32 (which uses 32-bit alignment for
64-bit integers). I don't know what the hardware definition is for TW_Command,
but ideally this would be expressed using only fixed-endian fixed-length
members and explicit padding.

    Arnd