[15/24] asus-wmi: Restrict debugfs interface when the kernel is locked down

Commit Message

David Howells April 5, 2017, 8:16 p.m. UTC

From: Matthew Garrett <[e-mail]>

We have no way of validating what all of the Asus WMI methods do on a given
machine - and there's a risk that some will allow hardware state to be
manipulated in such a way that arbitrary code can be executed in the
kernel, circumventing module loading restrictions.  Prevent that if the
kernel is locked down.

Signed-off-by: Matthew Garrett <[e-mail]>
Signed-off-by: David Howells <[e-mail]>
cc: [e-mail]
cc: [e-mail]
---

 drivers/platform/x86/asus-wmi.c |    9 +++++++++
 1 file changed, 9 insertions(+)


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Andy Shevchenko April 7, 2017, 10:25 a.m. UTC | #1

On Wed, Apr 5, 2017 at 11:16 PM, David Howells <[e-mail]> wrote:
> From: Matthew Garrett <[e-mail]>
>
> We have no way of validating what all of the Asus WMI methods do on a given
> machine - and there's a risk that some will allow hardware state to be
> manipulated in such a way that arbitrary code can be executed in the
> kernel, circumventing module loading restrictions.  Prevent that if the
> kernel is locked down.

> +       if (kernel_is_locked_down())
> +               return -EPERM;

It looks a bit fragile when responsility of whatever reasons kernel
can't serve become a driver burden.
Can we fix this in debugfs framework instead?

> +
>         err = asus_wmi_get_devstate(asus, asus->debug.dev_id, &retval);
>
>         if (err < 0)
> @@ -1916,6 +1919,9 @@ static int show_devs(struct seq_file *m, void *data)
>         int err;
>         u32 retval = -1;
>
> +       if (kernel_is_locked_down())
> +               return -EPERM;
> +
>         err = asus_wmi_set_devstate(asus->debug.dev_id, asus->debug.ctrl_param,
>                                     &retval);
>
> @@ -1940,6 +1946,9 @@ static int show_call(struct seq_file *m, void *data)
>         union acpi_object *obj;
>         acpi_status status;
>
> +       if (kernel_is_locked_down())
> +               return -EPERM;
> +
>         status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID,
>                                      1, asus->debug.method_id,
>                                      &input, &output);
>

David Howells April 7, 2017, 12:50 p.m. UTC | #2

Andy Shevchenko <[e-mail]> wrote:

> > From: Matthew Garrett <[e-mail]>
> >
> > We have no way of validating what all of the Asus WMI methods do on a given
> > machine - and there's a risk that some will allow hardware state to be
> > manipulated in such a way that arbitrary code can be executed in the
> > kernel, circumventing module loading restrictions.  Prevent that if the
> > kernel is locked down.
> 
> > +       if (kernel_is_locked_down())
> > +               return -EPERM;
> 
> It looks a bit fragile when responsility of whatever reasons kernel
> can't serve become a driver burden.
> Can we fix this in debugfs framework instead?

Fix it with debugfs how?  We can't offload the decision to userspace.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andy Shevchenko April 9, 2017, 11:10 a.m. UTC | #3

On Fri, Apr 7, 2017 at 3:50 PM, David Howells <[e-mail]> wrote:
> Andy Shevchenko <[e-mail]> wrote:
>
>> > From: Matthew Garrett <[e-mail]>
>> >
>> > We have no way of validating what all of the Asus WMI methods do on a given
>> > machine - and there's a risk that some will allow hardware state to be
>> > manipulated in such a way that arbitrary code can be executed in the
>> > kernel, circumventing module loading restrictions.  Prevent that if the
>> > kernel is locked down.
>>
>> > +       if (kernel_is_locked_down())
>> > +               return -EPERM;
>>
>> It looks a bit fragile when responsility of whatever reasons kernel
>> can't serve become a driver burden.
>> Can we fix this in debugfs framework instead?
>
> Fix it with debugfs how?  We can't offload the decision to userspace.

I mean to do at least similar like you have done for module
parameters. So, instead of putting above code to each attribute in
question make a special (marked) attribute instead and debugfs
framework will know how to deal with that.

David Howells April 10, 2017, 1:16 p.m. UTC | #4

Andy Shevchenko <[e-mail]> wrote:

> >> It looks a bit fragile when responsility of whatever reasons kernel
> >> can't serve become a driver burden.
> >> Can we fix this in debugfs framework instead?
> >
> > Fix it with debugfs how?  We can't offload the decision to userspace.
> 
> I mean to do at least similar like you have done for module
> parameters. So, instead of putting above code to each attribute in
> question make a special (marked) attribute instead and debugfs
> framework will know how to deal with that.

Hmmm...  It's tricky in that debugfs doesn't have any of its own structures,
but is entirely built on standard VFS ones, so finding somewhere to store the
information is going to be awkward.  One obvious solution is to entirely lock
down debugfs in secure boot more, but that might be a bit drastic.

Note that it's still going to be a driver burden to some extent anyway.  The
driver has to tell the core what needs to be restricted.

Further, I guess configfs needs attention also.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andy Shevchenko April 18, 2017, 6:06 a.m. UTC | #5

On Mon, Apr 10, 2017 at 4:16 PM, David Howells <[e-mail]> wrote:
> Andy Shevchenko <[e-mail]> wrote:
>
>> >> It looks a bit fragile when responsility of whatever reasons kernel
>> >> can't serve become a driver burden.
>> >> Can we fix this in debugfs framework instead?
>> >
>> > Fix it with debugfs how?  We can't offload the decision to userspace.
>>
>> I mean to do at least similar like you have done for module
>> parameters. So, instead of putting above code to each attribute in
>> question make a special (marked) attribute instead and debugfs
>> framework will know how to deal with that.
>
> Hmmm...  It's tricky in that debugfs doesn't have any of its own structures,
> but is entirely built on standard VFS ones, so finding somewhere to store the
> information is going to be awkward.

I see.

>  One obvious solution is to entirely lock
> down debugfs in secure boot more, but that might be a bit drastic.

But this sounds sane! debugFS for debugging, not for production. If
someone is using secure kernel it means pure production use (otherwise
one may do temporary hacks in kernel).
If one still needs debugfs in secure mode, it sounds to me as
architectural bug in code in question.

>
> Note that it's still going to be a driver burden to some extent anyway.  The
> driver has to tell the core what needs to be restricted.
>
> Further, I guess configfs needs attention also.

Ben Hutchings April 18, 2017, 2:34 p.m. UTC | #6

On Tue, 2017-04-18 at 09:06 +0300, Andy Shevchenko wrote:
> > On Mon, Apr 10, 2017 at 4:16 PM, David Howells <[e-mail]> wrote:
> > > > Andy Shevchenko <[e-mail]> wrote:
> > 
> > > > > It looks a bit fragile when responsility of whatever reasons kernel
> > > > > can't serve become a driver burden.
> > > > > Can we fix this in debugfs framework instead?
> > > > 
> > > > Fix it with debugfs how?  We can't offload the decision to userspace.
> > > 
> > > I mean to do at least similar like you have done for module
> > > parameters. So, instead of putting above code to each attribute in
> > > question make a special (marked) attribute instead and debugfs
> > > framework will know how to deal with that.
> > 
> > Hmmm...  It's tricky in that debugfs doesn't have any of its own structures,
> > but is entirely built on standard VFS ones, so finding somewhere to store the
> > information is going to be awkward.
> 
> I see.
> 
> >  One obvious solution is to entirely lock
> > down debugfs in secure boot more, but that might be a bit drastic.
> 
> But this sounds sane! debugFS for debugging, not for production. If
> someone is using secure kernel it means pure production use (otherwise
> one may do temporary hacks in kernel).
[...]

Production systems need instrumentation to understand performance
issues and any bugs that for whatever reason didn't show up in earlier
testing.  A number of interfaces for that have been added under
debugfs:

- tracing (now tracefs, but it's expected to appear under debugfs)
- dynamic_debug
- various ad-hoc statistics

So it's generally not going to be OK to turn off debugfs.  There will
probably need to be a distinction between believed-safe and unsafe
directories/files.

Ben.

David Howells April 18, 2017, 2:55 p.m. UTC | #7

Ben Hutchings <[e-mail]> wrote:

> - tracing (now tracefs, but it's expected to appear under debugfs)

Shouldn't this now appear under /sys/kernel/tracing/ ?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ben Hutchings April 18, 2017, 3:19 p.m. UTC | #8

On Tue, 2017-04-18 at 15:55 +0100, David Howells wrote:
> Ben Hutchings <[e-mail]> wrote:
> 
> > - tracing (now tracefs, but it's expected to appear under debugfs)
> 
> Shouldn't this now appear under /sys/kernel/tracing/ ?

True, but old tracing scripts didn't go away.

Ben.

David Howells April 18, 2017, 3:30 p.m. UTC | #9

Ben Hutchings <[e-mail]> wrote:

> So it's generally not going to be OK to turn off debugfs.  There will
> probably need to be a distinction between believed-safe and unsafe
> directories/files.

Any suggestion on how to mark this distinction?  I'd prefer not to modify
every read/write op associated with a debugfs file.  Modify
DEFINE_DEBUGFS_ATTRIBUTE() maybe?  And provide lockable variants of
debugfs_create_u8() and co.?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Howells April 18, 2017, 3:34 p.m. UTC | #10

Ben Hutchings <[e-mail]> wrote:

> > Shouldn't this now appear under /sys/kernel/tracing/ ?
> 
> True, but old tracing scripts didn't go away.

Conversion to a symlink would fix that.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ben Hutchings April 18, 2017, 5:39 p.m. UTC | #11

On Tue, 2017-04-18 at 16:30 +0100, David Howells wrote:
> Ben Hutchings <[e-mail]> wrote:
> 
> > So it's generally not going to be OK to turn off debugfs.  There will
> > probably need to be a distinction between believed-safe and unsafe
> > directories/files.
> 
> Any suggestion on how to mark this distinction?

I don't know.

> I'd prefer not to modify every read/write op associated with a
> debugfs file.

I think debugfs should be assumed unsafe by default.  So only the
believed-safe parts would need to be changed.

> Modify
> DEFINE_DEBUGFS_ATTRIBUTE() maybe?  And provide lockable variants of
> debugfs_create_u8() and co.?

That could help.

Ben.

Patch

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 8fe5890bf539..feef25076813 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1900,6 +1900,9 @@  static int show_dsts(struct seq_file *m, void *data)
 	int err;
 	u32 retval = -1;
 
+	if (kernel_is_locked_down())
+		return -EPERM;
+
 	err = asus_wmi_get_devstate(asus, asus->debug.dev_id, &retval);
 
 	if (err < 0)
@@ -1916,6 +1919,9 @@  static int show_devs(struct seq_file *m, void *data)
 	int err;
 	u32 retval = -1;
 
+	if (kernel_is_locked_down())
+		return -EPERM;
+
 	err = asus_wmi_set_devstate(asus->debug.dev_id, asus->debug.ctrl_param,
 				    &retval);
 
@@ -1940,6 +1946,9 @@  static int show_call(struct seq_file *m, void *data)
 	union acpi_object *obj;
 	acpi_status status;
 
+	if (kernel_is_locked_down())
+		return -EPERM;
+
 	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID,
 				     1, asus->debug.method_id,
 				     &input, &output);