| 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
| ||
|
Message-ID: <AM4PR0501MB21942F65C3F28F8AE9B22504B1440@AM4PR0501MB2194.eurprd05.prod.outlook.com>
Date: Wed, 25 Oct 2017 15:58:42 +0000
From: Oleksandr Shamray <oleksandrs@...lanox.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: "arnd@...db.de" <arnd@...db.de>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
"joel@....id.au" <joel@....id.au>,
"jiri@...nulli.us" <jiri@...nulli.us>,
"tklauser@...tanz.ch" <tklauser@...tanz.ch>,
"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
"mec@...ut.net" <mec@...ut.net>,
"Vadim Pasternak" <vadimp@...lanox.com>,
system-sw-low-level <system-sw-low-level@...lanox.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"openocd-devel-owner@...ts.sourceforge.net"
<openocd-devel-owner@...ts.sourceforge.net>,
"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"mchehab@...nel.org" <mchehab@...nel.org>,
Jiri Pirko <jiri@...lanox.com>
Subject: RE: [patch v9 1/4] drivers: jtag: Add JTAG core driver
> -----Original Message-----
> From: Greg KH [mailto:gregkh@...uxfoundation.org]
> Sent: Friday, October 20, 2017 5:54 PM
> To: Oleksandr Shamray <oleksandrs@...lanox.com>
> Cc: arnd@...db.de; linux-kernel@...r.kernel.org; linux-arm-
> kernel@...ts.infradead.org; devicetree@...r.kernel.org;
> openbmc@...ts.ozlabs.org; joel@....id.au; jiri@...nulli.us;
> tklauser@...tanz.ch; linux-serial@...r.kernel.org; mec@...ut.net; Vadim
> Pasternak <vadimp@...lanox.com>; system-sw-low-level <system-sw-low-
> level@...lanox.com>; robh+dt@...nel.org; openocd-devel-
> owner@...ts.sourceforge.net; linux-api@...r.kernel.org;
> davem@...emloft.net; mchehab@...nel.org; Jiri Pirko <jiri@...lanox.com>
> Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver
>
> On Fri, Oct 20, 2017 at 02:34:00PM +0000, Oleksandr Shamray wrote:
> > Hi Greg.
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@...uxfoundation.org]
> > > Sent: Friday, October 20, 2017 2:55 PM
> > > To: Oleksandr Shamray <oleksandrs@...lanox.com>
> > > Cc: arnd@...db.de; linux-kernel@...r.kernel.org; linux-arm-
> > > kernel@...ts.infradead.org; devicetree@...r.kernel.org;
> > > openbmc@...ts.ozlabs.org; joel@....id.au; jiri@...nulli.us;
> > > tklauser@...tanz.ch; linux-serial@...r.kernel.org; mec@...ut.net;
> > > Vadim Pasternak <vadimp@...lanox.com>; system-sw-low-level
> > > <system-sw-low- level@...lanox.com>; robh+dt@...nel.org;
> > > openocd-devel- owner@...ts.sourceforge.net;
> > > linux-api@...r.kernel.org; davem@...emloft.net; mchehab@...nel.org;
> > > Jiri Pirko <jiri@...lanox.com>
> > > Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver
> > >
> > > On Thu, Sep 21, 2017 at 12:25:29PM +0300, Oleksandr Shamray wrote:
> > > > +struct jtag {
> > > > + struct device *dev;
> > > > + struct cdev cdev;
> > >
> > > Why are you using a cdev here and not just a normal misc device?
> >
> > What the benefits to use misc instead of cdev?
>
> Less code, simpler logic, easier to review and understand, etc.
>
> Let me ask you, why use a cdev instead of a misc?
As I know misc device more applicable if we want to create one device f.e. /dev/jtag.
But in current case we can have more than one jtag device /dev/jtag0 ... /dev/jtagN.
So I decided to use cdev.
>
> > > I forgot if this is what you were doing before, sorry...
> > >
> > > > + int id;
> > > > + atomic_t open;
> > >
> > > Why do you need this?
> >
> > This counter used to avoid open at the same time by 2 or more users.
>
> But it isn't working :)
>
> And why do you care?
>
> > > > + const struct jtag_ops *ops;
> > > > + unsigned long priv[0] __aligned(ARCH_DMA_MINALIGN);
> > >
> > > Huh? Why is this needed to be dma aligned? Why not just use the
> > > private pointer in struct device?
> > >
> >
> > It is critical?
>
> You are saying it is, so you have to justify it. There is a pointer for you to use,
> don't make new ones for no reason, right?
>
You are right. Will remove.
> > > > +};
> > > > +
> > > > +static dev_t jtag_devt;
> > > > +static DEFINE_IDA(jtag_ida);
> > > > +
> > > > +void *jtag_priv(struct jtag *jtag) {
> > > > + return jtag->priv;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(jtag_priv);
> > > > +
> > > > +static u8 *jtag_copy_from_user(__u64 udata, unsigned long bit_size) {
> > > > + unsigned long size;
> > > > + void *kdata;
> > > > +
> > > > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > > > + kdata = memdup_user(u64_to_user_ptr(udata), size);
> > >
> > > You only use this once, why not just open-code it?
> >
> > I think it makes code more understandable.
>
> As a reviewer, I don't :)
Ok, I will fix :)
>
> > > > +
> > > > + return kdata;
> > > > +}
> > > > +
> > > > +static unsigned long jtag_copy_to_user(__u64 udata, u8 *kdata,
> > > > + unsigned long bit_size) {
> > > > + unsigned long size;
> > > > +
> > > > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > > > +
> > > > + return copy_to_user(u64_to_user_ptr(udata), (void *)(kdata),
> > > > +size);
> > >
> > > Same here, making this a separate function seems odd.
> >
> > Same, I think it makes code more understandable.
>
> But it doesn't.
>
Ok, I will fix :)
> > > > +
> > > > + if (jtag->ops->freq_set)
> > > > + err = jtag->ops->freq_set(jtag, value);
> > > > + else
> > > > + err = -EOPNOTSUPP;
> > > > + break;
> > > > +
> > > > + case JTAG_IOCRUNTEST:
> > > > + if (copy_from_user(&idle, (void *)arg,
> > > > + sizeof(struct jtag_run_test_idle)))
> > > > + return -ENOMEM;
> > > > + err = jtag_run_test_idle_op(jtag, &idle);
> > >
> > > Who validates the structure fields? Is that up to the individual
> > > jtag driver? Why not do it in the core correctly so that it only
> > > has to be done in one place and you do not have to audit every individual
> driver?
> >
> > Input parameters validated by jtag platform driver. I think it not critical.
>
> Not true at all. It is very critical. Remmeber, "All Input Is Evil!"
>
> You have to validate this. I as a reviewer have to find where you are validating
> this data to ensure bad things do not happen. I can't review that here, now I
> have to go and review all of the individual drivers, which is a major pain, don't
> you agree?
Agree. I will add input parameter checking here before call device driver.
>
> > > > + break;
> > > > +
> > > > + case JTAG_IOCXFER:
> > > > + if (copy_from_user(&xfer, (void *)arg,
> > > > + sizeof(struct jtag_xfer)))
> > > > + return -EFAULT;
> > > > +
> > > > + if (xfer.length >= JTAG_MAX_XFER_DATA_LEN)
> > > > + return -EFAULT;
> > > > +
> > > > + xfer_data = jtag_copy_from_user(xfer.tdio, xfer.length);
> > > > + if (!xfer_data)
> > > > + return -ENOMEM;
> > >
> > > Are you sure that's the correct error value?
> >
> > I think yes, but what you suggest?
>
> A fault happened, so -EFAULT, right?
>
Right.
> > [..]
> > > + .unlocked_ioctl = jtag_ioctl,
> > > + .open = jtag_open,
> > > + .release = jtag_release,
> > > +};
> >
> > add a compat_ioctl pointer here, after ensuring that all ioctl
> > commands are compatible between 32-bit and 64-bit user space.
> > [..]
>
> And if you do not, what happens? You shouldn't need it as there is no fixups
> necessary, or am I mistaken about that?
Yes, you are right. In code compat_ioctl called same function as in unlocked_ioctl.
So I can remove compat and system will always call unlocked_ioctl.
>
> > > > +static int jtag_open(struct inode *inode, struct file *file) {
> > > > + struct jtag *jtag = container_of(inode->i_cdev, struct jtag,
> > > > +cdev);
> > > > +
> > > > + if (atomic_read(&jtag->open)) {
> > > > + dev_info(NULL, "jtag already opened\n");
> > > > + return -EBUSY;
> > >
> > > Why do you care if multiple opens can happen?
> >
> > Jtag HW not support to using with multiple requests from different users. So
> we prohibit this.
>
> Why does the kernel care?
>
> And again, your implementation is broken, it's not actually doing this
> protection. I recommend just not doing it at all, but if you really are insisting
> on it, you have to get it correct :)
I will follow your recommendations and remove it.
>
> thanks,
>
> greg k-h
Thanks.
Oleksandr S
Powered by blists - more mailing lists