| 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: <CH2PR12MB4216ED068AD93C43B2C421A8AE050@CH2PR12MB4216.namprd12.prod.outlook.com>
Date: Wed, 29 Jan 2020 17:00:11 +0000
From: Vitor Soares <Vitor.Soares@...opsys.com>
To: Arnd Bergmann <arnd@...db.de>
CC: Jose Abreu <Jose.Abreu@...opsys.com>,
Joao Pinto <Joao.Pinto@...opsys.com>,
Boris Brezillon <bbrezillon@...nel.org>,
gregkh <gregkh@...uxfoundation.org>,
Wolfram Sang <wsa@...-dreams.de>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Mark Brown <broonie@...nel.org>,
"linux-i3c@...ts.infradead.org" <linux-i3c@...ts.infradead.org>
Subject: RE: [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev
Hi Arnd,
From: Arnd Bergmann <arnd@...db.de>
Date: Wed, Jan 29, 2020 at 14:30:56
> On Wed, Jan 29, 2020 at 1:17 PM Vitor Soares <Vitor.Soares@...opsys.com> wrote:
> >
> > +
> > +struct i3cdev_data {
> > + struct list_head list;
> > + struct i3c_device *i3c;
> > + struct cdev cdev;
> > + struct device *dev;
> > + int id;
> > +};
> > +
> > +static DEFINE_IDA(i3cdev_ida);
> > +static dev_t i3cdev_number;
> > +#define I3C_MINORS 16 /* 16 I3C devices supported for now */
> > +
> > +static LIST_HEAD(i3cdev_list);
> > +static DEFINE_SPINLOCK(i3cdev_list_lock);
>
> Please try to avoid arbitrarily limiting the number of devices you support.
Should I use all minors range instead?
>
> Searching through the list feels a little clumsy. If the i3c user interface is
> supposed to become a standard feature of the subsystem, it would seem
> appropriate to put a pointer into the device to simplify the lookup,
Do you mean i3c->dev ?
> or
> just embed the cdev inside of i3c_device.
I would prefer to have a pointer in i3c_device for i3cdev_data, but I see
others using it in drvdata.
>
> > +static int
> > +i3cdev_do_priv_xfer(struct i3c_device *dev, struct i3c_ioc_priv_xfer *xfers,
> > + unsigned int nxfers)
> > +{
> > + struct i3c_priv_xfer *k_xfers;
> > + u8 **data_ptrs;
> > + int i, ret = 0;
> > +
> > + k_xfers = kcalloc(nxfers, sizeof(*k_xfers), GFP_KERNEL);
> > + if (!k_xfers)
> > + return -ENOMEM;
> > +
> > + data_ptrs = kcalloc(nxfers, sizeof(*data_ptrs), GFP_KERNEL);
> > + if (!data_ptrs) {
> > + ret = -ENOMEM;
> > + goto err_free_k_xfer;
> > + }
>
> Maybe use a combined allocation to simplify the error handling?
Could you please provide an example?
>
> > + for (i = 0; i < nxfers; i++) {
> > + data_ptrs[i] = memdup_user((const u8 __user *)
> > + (uintptr_t)xfers[i].data,
> > + xfers[i].len);
>
> > + if (xfers[i].rnw) {
> > + if (copy_to_user((void __user *)(uintptr_t)xfers[i].data,
> > + data_ptrs[i], xfers[i].len))
>
> Use u64_to_user_ptr() here.
You are right, it wasn't available went I did it.
>
> > +
> > +static struct i3c_ioc_priv_xfer *
> > +i3cdev_get_ioc_priv_xfer(unsigned int cmd, struct i3c_ioc_priv_xfer *u_xfers,
> > + unsigned int *nxfers)
> > +{
> > + u32 tmp = _IOC_SIZE(cmd);
> > +
> > + if ((tmp % sizeof(struct i3c_ioc_priv_xfer)) != 0)
> > + return ERR_PTR(-EINVAL);
> > +
> > + *nxfers = tmp / sizeof(struct i3c_ioc_priv_xfer);
> > + if (*nxfers == 0)
> > + return NULL;
> > +
> > + return memdup_user(u_xfers, tmp);
> > +}
> > +
> > +static int
> > +i3cdev_ioc_priv_xfer(struct i3c_device *i3c, unsigned int cmd,
> > + struct i3c_ioc_priv_xfer *u_xfers)
> > +{
> > + struct i3c_ioc_priv_xfer *k_xfers;
> > + unsigned int nxfers;
> > + int ret;
> > +
> > + k_xfers = i3cdev_get_ioc_priv_xfer(cmd, u_xfers, &nxfers);
> > + if (IS_ERR_OR_NULL(k_xfers))
> > + return PTR_ERR(k_xfers);
> > +
> > + ret = i3cdev_do_priv_xfer(i3c, k_xfers, nxfers);
>
> The IS_ERR_OR_NULL() usage looks suspicious. It's generally
> better to avoid interfaces that require this. What does it mean to
> return NULL from i3cdev_get_ioc_priv_xfer() and turn that into
> success? Could you handle this condition in the caller instead,
> or turn it into an error?
In both cases something is not correct. I will turn both conditions to
return ERR_PTR(-EINVAL) and them just check if (IS_ERR(k_xfer)).
>
> > + /* Keep track of busses which have devices to add or remove later */
> > + res = bus_register_notifier(&i3c_bus_type, &i3c_notifier);
> > + if (res)
> > + goto out_unreg_class;
> > +
> > + /* Bind to already existing device without driver right away */
> > + i3c_for_each_dev(NULL, i3cdev_attach);
>
> The combination of the notifier and searching through the devices
> seems to be racy. What happens when a device appears just before
> or during the i3c_for_each_dev() traversal?
The i3c core is locked during this phase.
>
> What happens when a driver attaches to a device that is currently
> transferring data on the user interface?
>
It may lost references for inode and file. I need to guarantee there no
tranfer going on during the detach.
Do you have any suggestion?
> Is there any guarantee that the notifiers for attach and detach
> are serialized?
>
Sorry I didn't get this part.
> > +/**
> > + * struct i3c_ioc_priv_xfer - I3C SDR ioctl private transfer
> > + * @data: Holds pointer to userspace buffer with transmit data.
> > + * @len: Length of data buffer buffers, in bytes.
> > + * @rnw: encodes the transfer direction. true for a read, false for a write
> > + */
> > +struct i3c_ioc_priv_xfer {
> > + __u64 data;
> > + __u16 len;
> > + __u8 rnw;
> > + __u8 pad[5];
> > +};
> > +
> > +
> > +#define I3C_PRIV_XFER_SIZE(N) \
> > + ((((sizeof(struct i3c_ioc_priv_xfer)) * (N)) < (1 << _IOC_SIZEBITS)) \
> > + ? ((sizeof(struct i3c_ioc_priv_xfer)) * (N)) : 0)
> > +
> > +#define I3C_IOC_PRIV_XFER(N) \
> > + _IOC(_IOC_READ|_IOC_WRITE, I3C_DEV_IOC_MAGIC, 30, I3C_PRIV_XFER_SIZE(N))
>
> This looks like a reasonable ioctl definition, avoiding the usual problems
> with compat mode etc.
Do you think I should add more reserved fields for future?
>
> Arnd
>
> _______________________________________________
> linux-i3c mailing list
> linux-i3c@...ts.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Di3c&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=pv8xU_wOpDLOkwdQiuBDso73EKvNPX2jXLtBHDVWRFo&s=S-Tesk8Hi3Ok6y9d_ysocHXGt2dmnn-WcM0BxurcDdQ&e=
Thanks for your comments 😊
Best regards,
Vitor Soares
Powered by blists - more mailing lists