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

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

[David Howells]

From: Matthew Garrett <matthew.garrett@nebula.com>

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 <matthew.garrett@nebula.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: acpi4asus-user@lists.sourceforge.net
cc: platform-driver-x86@vger.kernel.org
---

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

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);

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

[Andy Shevchenko]

On Wed, Apr 5, 2017 at 11:16 PM, David Howells <dhowells@redhat.com> wrote:
> From: Matthew Garrett <matthew.garrett@nebula.com>
>
> 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);
>



-- 
With Best Regards,
Andy Shevchenko

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

[David Howells]

Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> > From: Matthew Garrett <matthew.garrett@nebula.com>
> >
> > 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

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

[Andy Shevchenko]

On Fri, Apr 7, 2017 at 3:50 PM, David Howells <dhowells@redhat.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>> > From: Matthew Garrett <matthew.garrett@nebula.com>
>> >
>> > 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.

-- 
With Best Regards,
Andy Shevchenko

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

[David Howells]

Andy Shevchenko <andy.shevchenko@gmail.com> 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

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

[Andy Shevchenko]

On Mon, Apr 10, 2017 at 4:16 PM, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.

-- 
With Best Regards,
Andy Shevchenko

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]

On Tue, 2017-04-18 at 09:06 +0300, Andy Shevchenko wrote:
> > On Mon, Apr 10, 2017 at 4:16 PM, David Howells <dhowells@redhat.com> wrote:
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> 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.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Howells]

Ben Hutchings <ben-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org> wrote:

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

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

David

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

On Tue, 2017-04-18 at 15:55 +0100, David Howells wrote:
> Ben Hutchings <ben@decadent.org.uk> 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.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Howells]

Ben Hutchings <ben@decadent.org.uk> 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

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

[David Howells]

Ben Hutchings <ben-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org> 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

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

On Tue, 2017-04-18 at 16:30 +0100, David Howells wrote:
> Ben Hutchings <ben-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org> 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.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]