linux-kernel - RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than globally

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]

Message-ID: <bb29662337484ad8bd2350e6b68133d3@ausx13mpc120.AMER.DELL.COM>
Date:   Wed, 31 Jan 2018 16:46:58 +0000
From:   <Mario.Limonciello@...l.com>
To:     <pali.rohar@...il.com>, <andy.shevchenko@...il.com>
CC:     <dvhart@...radead.org>, <linux-kernel@...r.kernel.org>,
        <platform-driver-x86@...r.kernel.org>
Subject: RE: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather
 than globally

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@...il.com]
> Sent: Wednesday, January 31, 2018 3:08 AM
> To: Andy Shevchenko <andy.shevchenko@...il.com>
> Cc: Limonciello, Mario <Mario_Limonciello@...l.com>; Darren Hart
> <dvhart@...radead.org>; LKML <linux-kernel@...r.kernel.org>; Platform Driver
> <platform-driver-x86@...r.kernel.org>
> Subject: Re: [PATCH] platform/x86: dell-laptop: Allocate buffer on heap rather than
> globally
> 
> On Tuesday 30 January 2018 20:14:26 Andy Shevchenko wrote:
> > >> -static void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> > >> +static void dell_set_arguments(struct calling_interface_buffer *buffer,
> > >> +                            u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> > >
> > > Hm... this function has too many parameters :-(
> > >
> > > What about following API?
> > >
> > > static struct calling_interface_buffer dell_set_arguments(u32 arg0, u32 arg1,
> u32 arg2, u32 arg3);
> > >
> > > It would return filled structure (not a pointer !)
> >
> > I do not think it's a good idea. Either we allocate request on a heap
> > and return a pointer, or we fill the struct with some data on spot.
> >
> > To naming:
> >
> > for allocation: ..._alloc_request()
> > for filling: _fill_request() / _prepare_request()
> >
> > or alike.
> >
> > _set_arguments() not good enough to me.
> 
> Ok. Then we need to stick with 5 arguments... What about name
> dell_fill_request()? E.g.
> 
>   struct calling_interface_buffer buffer;
>   dell_fill_request(&buffer, 0x2, 0, 0, 0);
>   ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> 

The other alternative is to just define the input of the structure immediately with
an initializer, no multi argument filler function.  Like this:

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index ab58373..e7a971b 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -469,13 +469,15 @@ static int dell_rfkill_set(void *data, bool blocked)
        int disable = blocked ? 1 : 0;
        unsigned long radio = (unsigned long)data;
        int hwswitch_bit = (unsigned long)data - 1;
-       struct calling_interface_buffer buffer;
+       struct calling_interface_buffer buffer = {CLASS_INFO,
+                                                 SELECT_RFKILL,
+                                                 {0, 0, 0, 0},
+                                                 {0, 0, 0, 0}};
        int hwswitch;
        int status;
        int ret;
 
-       dell_set_arguments(&buffer, 0, 0, 0, 0);
-       ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+       ret = dell_send_request(&buffer);
        if (ret)
                return ret;
        status = buffer.output[1];