linux-kernel - RE: [PATCH v6 09/14] platform/x86: dell-smbios: Introduce dispatcher for SMM calls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Oct 2017 19:14:53 +0000
From:   <Mario.Limonciello@...l.com>
To:     <pali.rohar@...il.com>
CC:     <dvhart@...radead.org>, <andy.shevchenko@...il.com>,
        <linux-kernel@...r.kernel.org>,
        <platform-driver-x86@...r.kernel.org>, <luto@...nel.org>,
        <quasisec@...gle.com>, <rjw@...ysocki.net>, <mjg59@...gle.com>,
        <hch@....de>, <greg@...ah.com>
Subject: RE: [PATCH v6 09/14] platform/x86: dell-smbios: Introduce dispatcher
 for SMM calls

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@...il.com]
> Sent: Tuesday, October 10, 2017 11:01 AM
> To: Limonciello, Mario <Mario_Limonciello@...l.com>
> Cc: dvhart@...radead.org; Andy Shevchenko <andy.shevchenko@...il.com>;
> LKML <linux-kernel@...r.kernel.org>; platform-driver-x86@...r.kernel.org;
> Andy Lutomirski <luto@...nel.org>; quasisec@...gle.com; rjw@...ysocki.net;
> mjg59@...gle.com; hch@....de; Greg KH <greg@...ah.com>
> Subject: Re: [PATCH v6 09/14] platform/x86: dell-smbios: Introduce dispatcher for
> SMM calls
> 
> On Monday 09 October 2017 17:51:47 Mario Limonciello wrote:
> >  static void dell_rfkill_query(struct rfkill *rfkill, void *data)
> >  {
> > -	struct calling_interface_buffer *buffer;
> >  	int radio = ((unsigned long)data & 0xF);
> >  	int hwswitch;
> >  	int status;
> >  	int ret;
> >
> > -	buffer = dell_smbios_get_buffer();
> > -
> > -	dell_smbios_send_request(17, 11);
> > -	ret = buffer->output[0];
> > +	ret = dell_send_request(17, 11, 0, 0, 0, 0);
> 
> Basically I do not like function which takes ten numeric parameters.
> Before in this code there was just function with 2 parameters, now there
> are lot of zero parameters, without information what which parameter
> means...
> 

In an earlier patch Darren wanted to keep dell_send_request around and
remove code that was duplicated multiple times to clear buffer, set arguments etc,
can you please comment on what exactly you would prefer?

It's very obvious if you look at the dell_send_request function what is happening.

> > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-
> smbios.c
> > index ade248646578..386130811a34 100644
> > --- a/drivers/platform/x86/dell-smbios.c
> > +++ b/drivers/platform/x86/dell-smbios.c
> > +void dell_smbios_get_smm_address(int *address, int *code)
> > +{
> > +	*address = da_command_address;
> > +	*code = da_command_code;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_smbios_get_smm_address);
> 
> Should not be this function in dell-smbios-smm driver instead of the
> dispatcher? It has in name "smm" and also is specific for smm driver...

That would mean parsing DA table in both drivers.  Since it was already
available in dell-smbios driver, it made sense to me to export it to the other
driver this way.

> 
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index 90d7e8e55e9b..b74f48d17116 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -641,13 +641,16 @@ static int dell_wmi_events_set_enabled(bool enable)
> >  	struct calling_interface_buffer *buffer;
> >  	int ret;
> >
> > -	buffer = dell_smbios_get_buffer();
> > +	buffer = (void *)get_zeroed_page(GFP_KERNEL);
> 
> It is still needed to ask for page? Is not enough to allocate memory via
> some *alloc function?

I think this particular one may be able to get away with less memory than a page.
I'll confirm.

> 
> > +	buffer->class = 17;
> > +	buffer->select = 3;
> >  	buffer->input[0] = 0x10000;
> >  	buffer->input[1] = 0x51534554;
> >  	buffer->input[3] = enable;
> > -	dell_smbios_send_request(17, 3);
> > -	ret = buffer->output[0];
> > -	dell_smbios_release_buffer();
> > +	ret = dell_smbios_call(buffer);
> > +	if (ret == 0)
> > +		ret = buffer->output[0];
> > +	free_page((unsigned long)buffer);
> >
> >  	return dell_smbios_error(ret);
> >  }
> 
> --
> Pali Rohár
> pali.rohar@...il.com