| Message ID | 20170112002910.3650-4-cyrilbur@gmail.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote: > This driver exposes a reserved chunk of BMC ram to userspace as well as > an ioctl interface to control the BMC<->HOST mapping of the LPC bus. > This allows for a communication channel between the BMC and the host Why not make this a UIO driver? Why does it have to be a character device? thanks, greg k-h
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; > +} 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. > +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. > +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
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. > 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. > > +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. > {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. > > > +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. > Again, use UIO, it will save you from yourself... > Thank-you! This is the first I've heard of UIO and I'll investigate furthur! Sincerely, Cyril > thanks, > > greg k-h
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
On Thu, 2017-01-12 at 11:30 +0100, Greg KH wrote: > > > 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. How so ? I mean, access_ok followed by __get/__put_user is still a classic, what's wrong with it ? The test performed by access_ok tests whether the address hits the kernel/userspace limit, that limit doesn't change afaik, so there is no race there, and avoids repeatedly testing it for series of subsequent __get_user or __put_user. What's wrong with them ? Cheers, Ben.
On Thu, 2017-01-12 at 21:16 +1100, Cyril Bur wrote: > My aim here was to only have one process playing with the LPC mapping > registers at a time. > > > Again, use UIO, it will save you from yourself... > > > > Thank-you! This is the first I've heard of UIO and I'll investigate > furthur! Greg, I don't think UIO is the answer here either. Note, this isn't an exploit so much as root shooting itself in the foot as this driver should never be accessed by anybody but root, but see below. This is a BMC, ie, the system controller of a x86 or POWER based system. The LPC controller controls the LPC bus which is mastered by the "host" (ie. the x86 or PPC) and acts as a slave on the BMC side. It has a bunch of registers that need to be configured in more/less system specific ways by the BMC, but more so, it has a pair of registers that allow "mapping" of a region of the BMC physical address space into the host address space. This is by definition dangerous to configure since it gives you a window to any part of the BMC, kernel space, any IOs, etc... however it needs to be configured by a userspace daemon which communicates with the host via a mailbox in order to map either different portions of the system flash controller address space or reserved memory. So in fact it should be done by the kernel, not userspace. What Cyril needs to do to make it more secure is: - For random register accesses, white list what registers specifically are allowed (and if necessary filter values). These registers aren't dangerous from the BMC perspective and need to be set appropriately for the host to operate correctly. - For the mapping of the LPC FW space <-> BMC space, use ioctl's to explicit establish the mapping to a portion of the flash (and nowhere else) or one of the known reserved memory areas. IE, dont have userspace just pass raw physical addresses through, but tell the kernel driver what portion (offset/size) of what area (flash space or reserved memory region) to configure the HW window for. Cheers, Ben.
On Thu, 2017-01-12 at 08:43 +0100, Greg KH wrote: > On Thu, Jan 12, 2017 at 11:29:09AM +1100, Cyril Bur wrote: > > This driver exposes a reserved chunk of BMC ram to userspace as > > well as > > an ioctl interface to control the BMC<->HOST mapping of the LPC > > bus. > > This allows for a communication channel between the BMC and the > > host > > Why not make this a UIO driver? Why does it have to be a character > device? See my other email (looks like I'm getting things in reverse order today ;-), basically it should just be some ioctl's, but what they do should really be done by the kernel as it controls external access to part of the chip physical address space. Cheers, Ben.
On Thu, Jan 12, 2017 at 09:27:47AM -0600, Benjamin Herrenschmidt wrote: > On Thu, 2017-01-12 at 11:30 +0100, Greg KH wrote: > > > > 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. > > How so ? I mean, access_ok followed by __get/__put_user is still a > classic, what's wrong with it ? No "normal" driver should do that, just call copy_to/from_user and be done with it. That way all of the proper locking and validation checks like this are done correctly for you. Why would a driver ever call the "raw" __get/__put_user functions? thanks, greg k-h
On Thu, 2017-01-12 at 17:00 +0100, Greg KH wrote: > > How so ? I mean, access_ok followed by __get/__put_user is still a > > classic, what's wrong with it ? > > No "normal" driver should do that, just call copy_to/from_user and be > done with it. That way all of the proper locking and validation checks > like this are done correctly for you. Why would a driver ever call the > "raw" __get/__put_user functions? I supposed historically it was considered faster for some things :-) Not a huge deal, and yes it's probably cleaner, I was just wondering what was "racy" about access_ok() that I might have missed... Cheers, Ben.
On Thu, Jan 12, 2017 at 10:07:33AM -0600, Benjamin Herrenschmidt wrote: > On Thu, 2017-01-12 at 17:00 +0100, Greg KH wrote: > > > How so ? I mean, access_ok followed by __get/__put_user is still a > > > classic, what's wrong with it ? > > > > No "normal" driver should do that, just call copy_to/from_user and be > > done with it. That way all of the proper locking and validation checks > > like this are done correctly for you. Why would a driver ever call the > > "raw" __get/__put_user functions? > > I supposed historically it was considered faster for some things :-) > > Not a huge deal, and yes it's probably cleaner, I was just wondering > what was "racy" about access_ok() that I might have missed... I think, you can change things after access_ok() happens, there used to be bugs in that area a few years ago. I think we fixed them by moving the offending drivers to use copy_*() instead. thanks, greg k-h
On Thu, Jan 12, 2017 at 09:35:15AM -0600, Benjamin Herrenschmidt wrote: > On Thu, 2017-01-12 at 21:16 +1100, Cyril Bur wrote: > > My aim here was to only have one process playing with the LPC mapping > > registers at a time. > > > > > Again, use UIO, it will save you from yourself... > > > > > > > Thank-you! This is the first I've heard of UIO and I'll investigate > > furthur! > > Greg, I don't think UIO is the answer here either. Note, this isn't an > exploit so much as root shooting itself in the foot as this driver > should never be accessed by anybody but root, but see below. > > This is a BMC, ie, the system controller of a x86 or POWER based > system. > > The LPC controller controls the LPC bus which is mastered by the "host" > (ie. the x86 or PPC) and acts as a slave on the BMC side. > > It has a bunch of registers that need to be configured in more/less > system specific ways by the BMC, but more so, it has a pair of > registers that allow "mapping" of a region of the BMC physical address > space into the host address space. > > This is by definition dangerous to configure since it gives you a > window to any part of the BMC, kernel space, any IOs, etc... however it > needs to be configured by a userspace daemon which communicates with > the host via a mailbox in order to map either different portions of the > system flash controller address space or reserved memory. > > So in fact it should be done by the kernel, not userspace. > > What Cyril needs to do to make it more secure is: > > - For random register accesses, white list what registers > specifically are allowed (and if necessary filter values). These > registers aren't dangerous from the BMC perspective and need to be set > appropriately for the host to operate correctly. > > - For the mapping of the LPC FW space <-> BMC space, use ioctl's to > explicit establish the mapping to a portion of the flash (and nowhere > else) or one of the known reserved memory areas. IE, dont have > userspace just pass raw physical addresses through, but tell the kernel > driver what portion (offset/size) of what area (flash space or reserved > memory region) to configure the HW window for. Yes, something more needs to be documented here, as what was proposed isn't acceptable at all. thanks, greg k-h
On Thu, 2017-01-12 at 09:35 -0600, Benjamin Herrenschmidt wrote: > Greg, I don't think UIO is the answer here either. Note, this isn't an > exploit so much as root shooting itself in the foot as this driver > should never be accessed by anybody but root, but see below. Reading back my previous email I realize that the lack of coffee made my prose a lot less clear than I intended it to be :-) I think some background is in order here, it will help whoever reviews this and Cyril, skip to the bottom to how I think you should articulate the driver. So on a bunch of server systems, you have a system controller typically known as a BMC controller all sort of things such as power to various elements, sometimes fans, often the system flash, etc... The Aspeed BMC family which is what is used on OpenPOWER machines and a number of x86 as well is typically connected to the host via an LPC bus. (among others). This is an ISA bus on steroids, it has IO and MEM/FW cycles (different address spaces, the subtle differences between MEM and FW can be ignored for the sake of this discussion). It's generally used by the BMC chip to provide the host with access to the system flash that contains the BIOS or other host firmware (via MEM/FW space) along with a number of SuperIO-style IOs (via IO space) such as UARTs, IPMI controllers, etc.... On the BMC chip side, this is all configured via a bunch of registers whose content is related to a given policy of what devices are exposed how and where on a given system, which is system/vendor specific, so we don't want to bolt that into the BMC kernel. So this started with a need to provide something nicer than /dev/mem for user space to configure these things. At that point, something like UIO could have still made sense. However... One important aspect of the configuration is how the MEM/FW space is exposed to the host (ie, the x86 or POWER). Some registers in that bridge can define a window remapping all or portion of the LPC MEM/FW space to a portion of the BMC internal bus, with no specific limits imposed in HW. As you can see, this can be pretty nasty. So for this, I think it makes sense to ensure that this window is configured by a kernel driver that can apply some serious sanity checks on what it is configured to map. In practice, user space wants to control this by flipping the mapping between essentially two types of portions of the BMC address space: - The flash space. This is a region of the BMC MMIO space that more/less directly maps the system flash (at least for reads, writes are somewhat more complicated). - One (or more) reserved area(s) of the BMC physical memory. The latter is needed for a number of things, such as avoiding letting the host manipulate the innards of the BMC flash controller via some evil backdoor, we want to do flash updates by routing the window to a portion of memory (under control of a mailbox protocol via some separate set of registers) which the host can use to write new data in bulk and then request the BMC to flash it. There are other uses, such as allowing the host to boot from an in-memory flash image rather than the one in flash (very handy for continuous integration and test, the BMC can just download new images), etc... So I think the best approach here is: - A pair of ioctls to read and write random registers in the LPC bridge for all the "generally configuration gunk". These have a filter to ensure that the registers controlling the above mapping cannot be accessed that way. - An ioctl to control the above mapping window. It takes as arguments the location in LPC space, the window type (flash vs. memory), for memory, maybe an ID (several windows to chose from), and the offset& size in the latter. The driver can enforce that the windows are one of the specially reserved areas of memory etc... - An mmap function to map those reserved windows into userspace so the daemon can communicate appropriately (only needed for the memory windows, the flash space is accessed via the normal /dev/mtd drivers) Greg, does that make sense ? Cheers, Ben.
On Thu, 2017-01-12 at 17:26 +0100, Greg KH wrote: > I think, you can change things after access_ok() happens, there used to > be bugs in that area a few years ago. I think we fixed them by moving > the offending drivers to use copy_*() instead. Ok, I'm surprised though, we still have a metric ton of code, especially in filesystems, who do access_ok. Generally the idea here is that the enforcement is done by the MMU normally via the permission in the page tables. access_ok() is simply needed to make sure we access the portion of the page tables representing user space, not kernel space, and that is a pretty fixed dichotomy. Anyway, this is academic, I agree that copy_to/from_... is nicer. Cheers, Ben.
On Thu, Jan 12, 2017 at 10:29:37AM -0600, Benjamin Herrenschmidt wrote: > So I think the best approach here is: > > - A pair of ioctls to read and write random registers in the > LPC bridge for all the "generally configuration gunk". These have a > filter to ensure that the registers controlling the above mapping > cannot be accessed that way. > > - An ioctl to control the above mapping window. It takes as > arguments the location in LPC space, the window type (flash vs. > memory), for memory, maybe an ID (several windows to chose from), and > the offset& size in the latter. The driver can enforce that the windows > are one of the specially reserved areas of memory etc... > > - An mmap function to map those reserved windows into userspace > so the daemon can communicate appropriately (only needed for the memory > windows, the flash space is accessed via the normal /dev/mtd drivers) > > Greg, does that make sense ? Yes, that makes a lot more sense to me. Thanks for writing it up, hopefully it survives into the next driver submission, otherwise I'll ask the same questions again due to not having a short-term memory at all :) thanks, greg k-h
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 64971baf11fa..8696627ce9d2 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -766,6 +766,15 @@ config PANEL_BOOT_MESSAGE An empty message will only clear the display at driver init time. Any other printf()-formatted message is valid with newline and escape codes. +config ASPEED_LPC_CTRL + depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON + bool "Build a driver to control the BMC to HOST LPC bus" + default "y" + ---help--- + Provides a driver to control BMC to HOST LPC mappings through + ioctl()s, the driver aso provides a read/write interface to a BMC ram + region where host LPC can be buffered. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig" diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 31983366090a..de1925a9c80b 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO) += echo/ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o obj-$(CONFIG_CXL_BASE) += cxl/ obj-$(CONFIG_PANEL) += panel.o +obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.o diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c new file mode 100644 index 000000000000..36ab718681a5 --- /dev/null +++ b/drivers/misc/aspeed-lpc-ctrl.c @@ -0,0 +1,269 @@ +/* + * Copyright 2016 IBM Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version + * 2 of the License, or (at your option) any later version. + */ + +#include <linux/mfd/syscon.h> +#include <linux/miscdevice.h> +#include <linux/mm.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/poll.h> +#include <linux/regmap.h> + +#include <linux/aspeed-lpc-ctrl.h> + +#define DEVICE_NAME "aspeed-lpc-ctrl" + +#define HICR7 0x8 +#define HICR8 0xc + +struct lpc_ctrl { + struct miscdevice miscdev; + struct regmap *regmap; + phys_addr_t base; + resource_size_t size; + uint32_t pnor_size; + uint32_t pnor_base; +}; + +static atomic_t lpc_ctrl_open_count = ATOMIC_INIT(0); + +static struct lpc_ctrl *file_lpc_ctrl(struct file *file) +{ + return container_of(file->private_data, struct lpc_ctrl, miscdev); +} + +static int lpc_ctrl_mmap(struct file *file, struct vm_area_struct *vma) +{ + struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file); + unsigned long vsize = vma->vm_end - vma->vm_start; + + if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size) + return -EINVAL; + + /* Other checks? */ + + if (remap_pfn_range(vma, vma->vm_start, + (lpc_ctrl->base >> PAGE_SHIFT) + vma->vm_pgoff, + vsize, vma->vm_page_prot)) + return -EAGAIN; + + return 0; +} + +static int lpc_ctrl_open(struct inode *inode, struct file *file) +{ + if (atomic_inc_return(&lpc_ctrl_open_count) == 1) + return 0; + + atomic_dec(&lpc_ctrl_open_count); + return -EBUSY; +} + +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; +} + +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)); + return rc; + case LPC_CTRL_IOCTL_UNMAP: + /* + * The top nibble in host lpc addresses references which + * firmware space, use space zero hence the & 0x0fff + */ + + rc = regmap_write(lpc_ctrl->regmap, HICR7, + lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff)); + if (rc) + return rc; + + rc = regmap_write(lpc_ctrl->regmap, HICR8, + (~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1)); + return rc; + } + + return -EINVAL; +} + +static int lpc_ctrl_release(struct inode *inode, struct file *file) +{ + atomic_dec(&lpc_ctrl_open_count); + return 0; +} + +static const struct file_operations lpc_ctrl_fops = { + .owner = THIS_MODULE, + .mmap = lpc_ctrl_mmap, + .open = lpc_ctrl_open, + .read = lpc_ctrl_read, + .write = lpc_ctrl_write, + .release = lpc_ctrl_release, + .unlocked_ioctl = lpc_ctrl_ioctl, +}; + +static int lpc_ctrl_probe(struct platform_device *pdev) +{ + struct lpc_ctrl *lpc_ctrl; + struct device *dev; + struct device_node *node; + struct resource resm; + int rc; + + if (!pdev || !pdev->dev.of_node) + return -ENODEV; + + dev = &pdev->dev; + + lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl), GFP_KERNEL); + if (!lpc_ctrl) + return -ENOMEM; + + node = of_parse_phandle(dev->of_node, "flash", 0); + if (!node) { + dev_err(dev, "Didn't find host pnor flash node\n"); + rc = -ENODEV; + goto out; + } + + rc = of_property_read_u32_index(node, "reg", 3, + &lpc_ctrl->pnor_size); + if (rc) + return rc; + rc = of_property_read_u32_index(node, "reg", 2, + &lpc_ctrl->pnor_base); + if (rc) + return rc; + + dev_info(dev, "Host PNOR base: 0x%08x size: 0x%08x\n", + lpc_ctrl->pnor_base, lpc_ctrl->pnor_size); + + dev_set_drvdata(&pdev->dev, lpc_ctrl); + + node = of_parse_phandle(dev->of_node, "memory-region", 0); + if (!node) { + dev_err(dev, "Didn't find reserved memory\n"); + rc = -EINVAL; + goto out; + } + + rc = of_address_to_resource(node, 0, &resm); + of_node_put(node); + if (rc) { + dev_err(dev, "Could address to resource\n"); + rc = -ENOMEM; + goto out; + } + + lpc_ctrl->size = resource_size(&resm); + lpc_ctrl->base = resm.start; + + lpc_ctrl->regmap = syscon_node_to_regmap( + pdev->dev.parent->of_node); + if (IS_ERR(lpc_ctrl->regmap)) { + dev_err(dev, "Couldn't get regmap\n"); + rc = -ENODEV; + goto out; + } + + lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR; + lpc_ctrl->miscdev.name = DEVICE_NAME; + lpc_ctrl->miscdev.fops = &lpc_ctrl_fops; + lpc_ctrl->miscdev.parent = dev; + rc = misc_register(&lpc_ctrl->miscdev); + if (rc) + dev_err(dev, "Unable to register device\n"); + else + dev_info(dev, "Loaded at 0x%08x (0x%08x)\n", + lpc_ctrl->base, lpc_ctrl->size); + +out: + return rc; +} + +static int lpc_ctrl_remove(struct platform_device *pdev) +{ + struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev); + + misc_deregister(&lpc_ctrl->miscdev); + lpc_ctrl = NULL; + + return 0; +} + +static const struct of_device_id lpc_ctrl_match[] = { + { .compatible = "aspeed,ast2400-lpc-ctrl" }, + { }, +}; + +static struct platform_driver lpc_ctrl_driver = { + .driver = { + .name = DEVICE_NAME, + .of_match_table = lpc_ctrl_match, + }, + .probe = lpc_ctrl_probe, + .remove = lpc_ctrl_remove, +}; + +module_platform_driver(lpc_ctrl_driver); + +MODULE_DEVICE_TABLE(of, lpc_ctrl_match); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>"); +MODULE_DESCRIPTION("Linux device interface to control LPC bus"); diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h b/include/uapi/linux/aspeed-lpc-ctrl.h new file mode 100644 index 000000000000..c5f1caf827ac --- /dev/null +++ b/include/uapi/linux/aspeed-lpc-ctrl.h @@ -0,0 +1,25 @@ +/* + * Copyright 2016 IBM Corp. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version + * 2 of the License, or (at your option) any later version. + */ + +#ifndef _UAPI_LINUX_LPC_CTRL_H +#define _UAPI_LINUX_LPC_CTRL_H + +#include <linux/ioctl.h> + +struct lpc_mapping { + uint32_t hostaddr; + uint32_t size; +}; + +#define __LPC_CTRL_IOCTL_MAGIC 0xb2 +#define LPC_CTRL_IOCTL_SIZE _IOR(__LPC_CTRL_IOCTL_MAGIC, 0x00, uint32_t) +#define LPC_CTRL_IOCTL_MAP _IOW(__LPC_CTRL_IOCTL_MAGIC, 0x01, struct lpc_mapping) +#define LPC_CTRL_IOCTL_UNMAP _IO(__LPC_CTRL_IOCTL_MAGIC, 0x02) + +#endif /* _UAPI_LINUX_LPC_CTRL_H */
This driver exposes a reserved chunk of BMC ram to userspace as well as an ioctl interface to control the BMC<->HOST mapping of the LPC bus. This allows for a communication channel between the BMC and the host Signed-off-by: Cyril Bur <cyrilbur@gmail.com> --- drivers/misc/Kconfig | 9 ++ drivers/misc/Makefile | 1 + drivers/misc/aspeed-lpc-ctrl.c | 269 +++++++++++++++++++++++++++++++++++ include/uapi/linux/aspeed-lpc-ctrl.h | 25 ++++ 4 files changed, 304 insertions(+) create mode 100644 drivers/misc/aspeed-lpc-ctrl.c create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h