Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751565AbdHOLQK (ORCPT ); Tue, 15 Aug 2017 07:16:10 -0400 Received: from mail-oi0-f65.google.com ([209.85.218.65]:33449 "EHLO mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750888AbdHOLQI (ORCPT ); Tue, 15 Aug 2017 07:16:08 -0400 MIME-Version: 1.0 In-Reply-To: <1502791207-26951-2-git-send-email-oleksandrs@mellanox.com> References: <1502791207-26951-1-git-send-email-oleksandrs@mellanox.com> <1502791207-26951-2-git-send-email-oleksandrs@mellanox.com> From: Arnd Bergmann Date: Tue, 15 Aug 2017 13:16:06 +0200 X-Google-Sender-Auth: ChDZz4Mpao2Jh-pAEcbxkB383cE Message-ID: Subject: Re: [patch v3 1/3] drivers: jtag: Add JTAG core driver To: Oleksandr Shamray Cc: gregkh , Linux Kernel Mailing List , Linux ARM , devicetree@vger.kernel.org, OpenBMC Maillist , Joel Stanley , =?UTF-8?B?SmnFmcOtIFDDrXJrbw==?= , Tobias Klauser , linux-serial@vger.kernel.org, mec@shout.net, vadimp@maellanox.com, system-sw-low-level@mellanox.com, Rob Herring , openocd-devel-owner@lists.sourceforge.net, Jiri Pirko Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2631 Lines: 77 On Tue, Aug 15, 2017 at 12:00 PM, Oleksandr Shamray 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