Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver
From: Greg KH <hidden>
Date: 2017-01-12 07:47:50
Also in:
openbmc
On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote:
quoted hunk
+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; +}
Those functions don't actually do anything, so why even include them? And don't call access_ok(), it's racy and no driver should ever do that.
quoted hunk
+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!
{sigh}
Your assignment is to go find a whiteboard/blackboard/whatever and write
on it 100 times:
All input is evil.
I want to see the picture of that before you send any more kernel patches.
quoted hunk
+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? Again, use UIO, it will save you from yourself... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html