From: Oleksandr Shamray <oleksandrs@mellanox.com>
To: Arnd Bergmann <arnd@arndb.de>
CC: gregkh <gregkh@linuxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        OpenBMC Maillist <openbmc@lists.ozlabs.org>,
        Joel Stanley <joel@jms.id.au>,
        =?utf-8?B?SmnFmcOtIFDDrXJrbw==?= <jiri@resnulli.us>,
        Tobias Klauser <tklauser@distanz.ch>,
        "linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
        "mec@shout.net" <mec@shout.net>,
        "vadimp@maellanox.com" <vadimp@maellanox.com>,
        system-sw-low-level <system-sw-low-level@mellanox.com>,
        Rob Herring <robh+dt@kernel.org>,
        "openocd-devel-owner@lists.sourceforge.net" 
        <openocd-devel-owner@lists.sourceforge.net>,
        Jiri Pirko <jiri@mellanox.com>
Subject: RE: [patch v3 1/3] drivers: jtag: Add JTAG core driver
Thread-Topic: [patch v3 1/3] drivers: jtag: Add JTAG core driver
Thread-Index: AQHTFa1XL6LNiNTEHUiQ8rcLbWol1aKFRGsAgAHC42A=
Date: Wed, 16 Aug 2017 14:24:16 +0000
Message-ID: <AM4PR0501MB2194CBCC82F7EB67FE73304AB1820@AM4PR0501MB2194.eurprd05.prod.outlook.com>
References: <1502791207-26951-1-git-send-email-oleksandrs@mellanox.com>
 <1502791207-26951-2-git-send-email-oleksandrs@mellanox.com>
 <CAK8P3a3DutrfhGrd9MnDFbzhfvnZXSyrV2RbTgrFGgpbAs=H0w@mail.gmail.com>
In-Reply-To: <CAK8P3a3DutrfhGrd9MnDFbzhfvnZXSyrV2RbTgrFGgpbAs=H0w@mail.gmail.com>
Accept-Language: en-US, uk-UA
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Aug 2017 14:24:16.2828
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR05MB1757
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id v7GEOSiT017495
Content-Length: 3815
Lines: 102

> -----Original Message-----
> From: arndbergmann@gmail.com [mailto:arndbergmann@gmail.com] On
> Behalf Of Arnd Bergmann
> Sent: Tuesday, August 15, 2017 2:16 PM
> To: Oleksandr Shamray <oleksandrs@mellanox.com>
> Cc: gregkh <gregkh@linuxfoundation.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>;
> devicetree@vger.kernel.org; OpenBMC Maillist <openbmc@lists.ozlabs.org>;
> Joel Stanley <joel@jms.id.au>; Jiří Pírko <jiri@resnulli.us>; Tobias Klauser
> <tklauser@distanz.ch>; linux-serial@vger.kernel.org; mec@shout.net;
> vadimp@maellanox.com; system-sw-low-level <system-sw-low-
> level@mellanox.com>; Rob Herring <robh+dt@kernel.org>; openocd-devel-
> owner@lists.sourceforge.net; Jiri Pirko <jiri@mellanox.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@mellanox.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.

Thanks,  I think using __u64 instead of  pointer will be a good solution.

> 
> > +       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?

is_open flag protects from the opening file more than one time.

> 
> > + * @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.
> 

Thank. I will do it.

>         Arnd