linux-kernel - Re: [PATCH 5/7] bcm-vk: add bcm

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]

Date:   Tue, 27 Aug 2019 15:54:36 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Scott Branden <scott.branden@...adcom.com>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        David Brown <david.brown@...aro.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Shuah Khan <shuah@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>,
        BCM Kernel Feedback <bcm-kernel-feedback-list@...adcom.com>,
        Olof Johansson <olof@...om.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Colin Ian King <colin.king@...onical.com>,
        Kees Cook <keescook@...omium.org>,
        Takashi Iwai <tiwai@...e.de>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH 5/7] bcm-vk: add bcm_vk UAPI

On Thu, Aug 22, 2019 at 9:25 PM Scott Branden
<scott.branden@...adcom.com> wrote:
>
> Add user space api for bcm-vk driver.

> +
> +struct vk_metadata {
> +       /* struct version, always backwards compatible */
> +       __u32 version;
> +
> +       /* Version 0 fields */
> +       __u32 card_status;
> +#define VK_CARD_STATUS_FASTBOOT_READY BIT(0)
> +#define VK_CARD_STATUS_FWLOADER_READY BIT(1)
> +
> +       __u32 firmware_version;
> +       __u32 fw_status;
> +       /* End version 0 fields */
> +
> +       __u64 reserved[14];
> +       /* Total of 16*u64 for all versions */
> +};

I'd suggest getting rid of the API version fields, just leave the version 0
fields here and add a new structure + ioctl if you need other
fields

Versioning usually just adds complexity and is hard to get right.

> +struct vk_access {
> +       __u8 barno;     /* BAR number to use */
> +       __u8 type;      /* Type of access */
> +#define VK_ACCESS_READ 0
> +#define VK_ACCESS_WRITE 1
> +       __u32 len;      /* length of data */
> +       __u64 offset;   /* offset in BAR */
> +       __u32 *data;    /* where to read/write data to */
> +};

The pointer in the last member makes the structure incompatible between
32-bit and 64-bit user space. You could work around that using a __u64
member and turning that into a pointer using the u64_to_user_ptr()
macro in the driver in a portable way.

However, since this seems to be a read/write type interface, maybe
it's better to just use read/write file operations.

I also wonder if the interface should be on a higher abstraction level
here.

      Arnd