[3/4] drivers/misc: Add ASpeed LPC control driver
diff mbox

Message ID 20170112002910.3650-4-cyrilbur@gmail.com
State Awaiting Upstream
Headers show

Commit Message

Cyril Bur Jan. 12, 2017, 12:29 a.m. UTC
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

Comments

Greg KH Jan. 12, 2017, 7:43 a.m. UTC | #1
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
Greg KH Jan. 12, 2017, 7:47 a.m. UTC | #2
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
Cyril Bur Jan. 12, 2017, 10:16 a.m. UTC | #3
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
Greg KH Jan. 12, 2017, 10:30 a.m. UTC | #4
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
Benjamin Herrenschmidt Jan. 12, 2017, 3:27 p.m. UTC | #5
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.
Benjamin Herrenschmidt Jan. 12, 2017, 3:35 p.m. UTC | #6
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.
Benjamin Herrenschmidt Jan. 12, 2017, 3:36 p.m. UTC | #7
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.
Greg KH Jan. 12, 2017, 4 p.m. UTC | #8
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
Benjamin Herrenschmidt Jan. 12, 2017, 4:07 p.m. UTC | #9
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.
Greg KH Jan. 12, 2017, 4:26 p.m. UTC | #10
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
Greg KH Jan. 12, 2017, 4:27 p.m. UTC | #11
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
Benjamin Herrenschmidt Jan. 12, 2017, 4:29 p.m. UTC | #12
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.
Benjamin Herrenschmidt Jan. 12, 2017, 4:31 p.m. UTC | #13
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.
Greg KH Jan. 12, 2017, 5:27 p.m. UTC | #14
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

Patch
diff mbox

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 */