> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On
> Behalf Of Arnd Bergmann
> Sent: Tuesday, August 15, 2017 2:16 PM
> To: Oleksandr Shamray <[email protected]>
> Cc: gregkh <[email protected]>; Linux Kernel Mailing List <linux-
> [email protected]>; Linux ARM <[email protected]>;
> [email protected]; OpenBMC Maillist <[email protected]>;
> Joel Stanley <[email protected]>; Jiří Pírko <[email protected]>; Tobias Klauser
> <[email protected]>; [email protected]; [email protected];
> [email protected]; system-sw-low-level <system-sw-low-
> [email protected]>; Rob Herring <[email protected]>; openocd-devel-
> [email protected]; Jiri Pirko <[email protected]>
> Subject: Re: [patch v3 1/3] drivers: jtag: Add JTAG core driver
>
> On Tue, Aug 15, 2017 at 12:00 PM, Oleksandr Shamray
> <[email protected]> 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
