lkml.org 
[lkml]   [2019]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V3 04/12] misc: xilinx_sdfec: Add open, close and ioctl
Date
Hi Greg,

Please find inline comments below.

Regards
Dragan

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday 2 May 2019 18:23
> To: Dragan Cvetic <draganc@xilinx.com>
> Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org;
> mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com>
> Subject: Re: [PATCH V3 04/12] misc: xilinx_sdfec: Add open, close and ioctl
>
> On Sat, Apr 27, 2019 at 11:04:58PM +0100, Dragan Cvetic wrote:
> > +static int xsdfec_dev_open(struct inode *iptr, struct file *fptr)
> > +{
> > + struct xsdfec_dev *xsdfec;
> > +
> > + xsdfec = container_of(iptr->i_cdev, struct xsdfec_dev, xsdfec_cdev);
> > +
> > + if (!atomic_dec_and_test(&xsdfec->open_count)) {
>
> Why do you care about this?
>
> And do you really think it matters? What are you trying to protect from
> here?

There is a request to increase the driver security.
It is acceptable for us for now, even with non-perfections (will not be protected if opened twice with dup() or fork()).
This is covered in the documentation.
>
> > + atomic_inc(&xsdfec->open_count);
> > + return -EBUSY;
> > + }
> > +
> > + fptr->private_data = xsdfec;
> > + return 0;
> > +}
> > +
> > +static int xsdfec_dev_release(struct inode *iptr, struct file *fptr)
> > +{
> > + struct xsdfec_dev *xsdfec;
> > +
> > + xsdfec = container_of(iptr->i_cdev, struct xsdfec_dev, xsdfec_cdev);
> > +
> > + atomic_inc(&xsdfec->open_count);
>
> You increment a number when the device is closed?
>
> You are trying to make it hard to maintain this code over time :)
>
>
> > + return 0;
> > +}
> > +
> > +static long xsdfec_dev_ioctl(struct file *fptr, unsigned int cmd,
> > + unsigned long data)
> > +{
> > + struct xsdfec_dev *xsdfec = fptr->private_data;
> > + void __user *arg = NULL;
> > + int rval = -EINVAL;
> > + int err = 0;
> > +
> > + if (!xsdfec)
> > + return rval;
> > +
> > + if (_IOC_TYPE(cmd) != XSDFEC_MAGIC)
> > + return -ENOTTY;
> > +
> > + /* check if ioctl argument is present and valid */
> > + if (_IOC_DIR(cmd) != _IOC_NONE) {
> > + arg = (void __user *)data;
> > + if (!arg) {
> > + dev_err(xsdfec->dev,
> > + "xilinx sdfec ioctl argument is NULL Pointer");
>
> You just created a way for userspace to spam the kernel log, please do
> not do that :(

Will be removed.

>
>
>
> > + return rval;
> > + }
> > + }
> > +
> > + if (err) {
> > + dev_err(xsdfec->dev, "Invalid xilinx sdfec ioctl argument");
> > + return -EFAULT;
>
> Wrong error, you did not have a memory fault.

Absolutely useless code. Will be removed. Thanks.

>
> Again, you just created a way to spam the kernel log by a user :(
>
> thanks,
>
> greg k-h

\
 
 \ /
  Last update: 2019-05-03 18:46    [W:0.817 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site