linux-kernel - Re: [patch v3 1/3] drivers: jtag: Add JTAG core driver

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, 15 Aug 2017 13:16:06 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Oleksandr Shamray <oleksandrs@...lanox.com>
Cc:     gregkh <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        devicetree@...r.kernel.org,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        Joel Stanley <joel@....id.au>,
        Jiří Pírko <jiri@...nulli.us>,
        Tobias Klauser <tklauser@...tanz.ch>,
        linux-serial@...r.kernel.org, mec@...ut.net, vadimp@...llanox.com,
        system-sw-low-level@...lanox.com, Rob Herring <robh+dt@...nel.org>,
        openocd-devel-owner@...ts.sourceforge.net,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [patch v3 1/3] drivers: jtag: Add JTAG core driver

On Tue, Aug 15, 2017 at 12:00 PM, Oleksandr Shamray
<oleksandrs@...lanox.com> wrote:

> +       case JTAG_IOCXFER:
> +               if (copy_from_user(&xfer, varg, sizeof(struct jtag_xfer)))
> +                       return -EFAULT;
> +
> +               if (xfer.length >= JTAG_MAX_XFER_DATA_LEN)
> +                       return -EFAULT;
> +
> +               user_tdio_data = xfer.tdio;
> +               xfer.tdio = jtag_copy_from_user((void __user *)user_tdio_data,
> +                                               xfer.length);
> +               if (!xfer.tdio)
> +                       return -ENOMEM;

This is not safe for 32-bit processes on 64-bit kernels, since the
structure layout differs for the pointer member. It's better to always
use either rework the structure to avoid the pointer, or to use a
__u64 member to store it, and then use u64_to_user_ptr()
to convert it in the kernel.

> +       case JTAG_GIOCSTATUS:
> +               if (jtag->ops->status_get)
> +                       err = jtag->ops->status_get(jtag,
> +                                               (enum jtag_endstate *)&value);

This pointer cast is also not safe, as an enum might have a different
size than the 'value' variable that is not an enum. I think you need to
change the argument type for the callback, or maybe use another
intermediate.

> +static int jtag_open(struct inode *inode, struct file *file)
> +{
> +       struct jtag *jtag = container_of(inode->i_cdev, struct jtag, cdev);
> +
> +       spin_lock(&jtag->lock);
> +
> +       if (jtag->is_open) {
> +               dev_info(NULL, "jtag already opened\n");
> +               spin_unlock(&jtag->lock);
> +               return -EBUSY;
> +       }
> +
> +       jtag->is_open = true;
> +       file->private_data = jtag;
> +       spin_unlock(&jtag->lock);
> +       return 0;
> +}

Does the 'is_open' flag protect you from something that doesn't
also happen after a 'dup()' call on the file descriptor?

> + * @mode: access mode
> + * @type: transfer type
> + * @direction: xfer direction
> + * @length: xfer bits len
> + * @tdio : xfer data array
> + * @endir: xfer end state
> + *
> + * Structure represents interface to Aspeed JTAG device for jtag sdr xfer
> + * execution.
> + */
> +struct jtag_xfer {
> +       __u8    mode;
> +       __u8    type;
> +       __u8    direction;
> +       __u32   length;
> +       __u8    *tdio;
> +       __u8    endstate;
> +};

As mentioned above, the pointer in here makes it incompatible. Also,
you should reorder the members to avoid the implied padding.
Moving the 'endstate' before 'length' is sufficient.

        Arnd