On Thu, Jan 12, 2017 at 09:16:03PM +1100, Cyril Bur wrote:
> On Thu, 2017-01-12 at 08:47 +0100, Greg KH wrote:
> > On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
> > > +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> > > + size_t count, loff_t *ppos)
> > > +{
> > > + if (!access_ok(VERIFY_WRITE, buf, count))
> > > + return -EFAULT;
> > > +
> > > + return -EPERM;
> > > +}
> > > +
> > > +static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
> > > + size_t count, loff_t *ppos)
> > > +{
> > > + if (!access_ok(VERIFY_READ, buf, count))
> > > + return -EFAULT;
> > > +
> > > + return -EPERM;
> > > +}
> >
>
> Hello Greg,
>
> > Those functions don't actually do anything, so why even include them?
> >
>
> Apologies, I should be more careful with what I send.
Hm, that implies you never tested what you sent, nor intended for us to
merge it, an odd thing for you to do :)
> > And don't call access_ok(), it's racy and no driver should ever do that.
> >
>
> Oh, duly noted. I'll be sure to check out how and why. Perhaps it
> would be wise that no driver actually do that, I'm quite sure I used
> other drivers as examples of best practice.
You did? Please point me at that code so we can fix them up properly.
Cargo-cult coding is not a good thing, but it happens, so if we can at
least provide clean code to fixate on, it's good overall for everyone.
> > > +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> > > + unsigned long param)
> > > +{
> > > + long rc;
> > > + struct lpc_mapping map;
> > > + struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > > + void __user *p = (void __user *)param;
> > > +
> > > + switch (cmd) {
> > > + case LPC_CTRL_IOCTL_SIZE:
> > > + return copy_to_user(p, &lpc_ctrl->size,
> > > + sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> > > + case LPC_CTRL_IOCTL_MAP:
> > > + if (copy_from_user(&map, p, sizeof(map)))
> > > + return -EFAULT;
> > > +
> > > +
> > > + /*
> > > + * The top half of HICR7 is the MSB of the BMC address of the
> > > + * mapping.
> > > + * The bottom half of HICR7 is the MSB of the HOST LPC
> > > + * firmware space address of the mapping.
> > > + *
> > > + * The 1 bits in the top of half of HICR8 represent the bits
> > > + * (in the requested address) that should be ignored and
> > > + * replaced with those from the top half of HICR7.
> > > + * The 1 bits in the bottom half of HICR8 represent the bits
> > > + * (in the requested address) that should be kept and pass
> > > + * into the BMC address space.
> > > + */
> > > +
> > > + rc = regmap_write(lpc_ctrl->regmap, HICR7,
> > > + (lpc_ctrl->base | (map.hostaddr >> 16)));
> > > + if (rc)
> > > + return rc;
> > > +
> > > + rc = regmap_write(lpc_ctrl->regmap, HICR8,
> > > + (~(map.size - 1)) | ((map.size >> 16) - 1));
> >
> > Look Ma, a kernel exploit!
> >
>
> So 'evil' input here could allow the host to control the bmc,
> personally I file that under 'stupid' input. Also, stupid but not
> accidental, I don't believe one could accidentally come up with such
> input, although you never know what silly things human beings sometimes
> do. For what its worth, I'm not even sure that can happen but I'll
> grant you the benifit of the doubt.
I think you didn't get the main point here, again:
> > {sigh}
> >
> > Your assignment is to go find a whiteboard/blackboard/whatever and write
> > on it 100 times:
> > All input is evil.
You can NEVER trust any input values sent to the kernel, you have to
ALWAYS verify they are within certain safe ranges.
> > I want to see the picture of that before you send any more kernel patches.
> >
> > > +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> > > +{
> > > + atomic_dec(&lpc_ctrl_open_count);
> >
> > Totally unneeded and unnecessary, why do you care?
> >
>
> My aim here was to only have one process playing with the LPC mapping
> registers at a time.
Why? Who cares? You don't have internal state being kept by the
driver, so it shouldn't matter.
And again, don't treat an atomic variable as a lock, use a real lock for
the task, it works better, and is the correct thing to do.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
