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
| ||
|
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
Powered by blists - more mailing lists