linux-kernel - Re: [PATCH 5/5] staging: r8188eu: Use vsprintf extension %phCX to format a copy_to

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: <YSkEWBbOJmNSI4zn@kroah.com>
Date:   Fri, 27 Aug 2021 17:27:20 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Joe Perches <joe@...ches.com>
Cc:     linux-kernel@...r.kernel.org,
        Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        linux-staging@...ts.linux.dev
Subject: Re: [PATCH 5/5] staging: r8188eu: Use vsprintf extension %phCX to
 format a copy_to_user string

On Fri, Aug 27, 2021 at 08:23:31AM -0700, Joe Perches wrote:
> On Fri, 2021-08-27 at 10:42 +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 26, 2021 at 11:43:05AM -0700, Joe Perches wrote:
> > > This reduces object size without changing the string content.
> > > 
> > > compiled x86-64 defconfig w/ r8188eu and gcc 10.3.0
> > > 
> > > $ size drivers/staging/r8188eu/os_dep/ioctl_linux.o*
> > >    text	   data	    bss	    dec	    hex	filename
> > >   72556	   1548	      0	  74104	  12178	drivers/staging/r8188eu/os_dep/ioctl_linux.o.new
> > >   72758	   1548	      0	  74306	  12242	drivers/staging/r8188eu/os_dep/ioctl_linux.o.old
> > > 
> > > Signed-off-by: Joe Perches <joe@...ches.com>
> > > ---
> > >  drivers/staging/r8188eu/os_dep/ioctl_linux.c | 9 +++------
> > >  1 file changed, 3 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > > index ab4a9200f0791..048164659d872 100644
> > > --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > > +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > > @@ -2907,10 +2907,8 @@ static int rtw_p2p_get_groupid(struct net_device *dev,
> > >  	struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> > >  	struct wifidirect_info *pwdinfo = &padapter->wdinfo;
> > >  
> > > 
> > > -	sprintf(extra, "\n%.2X:%.2X:%.2X:%.2X:%.2X:%.2X %s",
> > > -		pwdinfo->groupid_info.go_device_addr[0], pwdinfo->groupid_info.go_device_addr[1],
> > > -		pwdinfo->groupid_info.go_device_addr[2], pwdinfo->groupid_info.go_device_addr[3],
> > > -		pwdinfo->groupid_info.go_device_addr[4], pwdinfo->groupid_info.go_device_addr[5],
> > > +	sprintf(extra, "\n%pM %s",
> > > +		pwdinfo->groupid_info.go_device_addr,
> > >  		pwdinfo->groupid_info.ssid);
> > 
> > We can just use the lower-case one here, no need for a new modifier just
> > for something like this (i.e. log file output)
> 
> That was just a trivial conversion of log file output and is lower case
> as log file output is not ABI.
> 
> The copy_to_user bit (2nd diff block) is nominally an ABI and is upper case.
> IMO at a minimum, it's bad form to change it.
> 
> @@ -3075,8 +3073,7 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
>         if (!blnMatch)
>                 sprintf(go_devadd_str, "\n\ndev_add = NULL");
>         else
> -               sprintf(go_devadd_str, "\ndev_add =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
> -                       attr_content[0], attr_content[1], attr_content[2], attr_content[3], attr_content[4], attr_content[5]);
> +               sprintf(go_devadd_str, "\ndev_add =%6phCX", attr_content);
>  
>         if (copy_to_user(wrqu->data.pointer, go_devadd_str, 10 + 17))
>                 return -EFAULT;

That looks like a horrible driver-specific api that no one will really
be using and will be removed from the tree soon.  So it can be changed,
no need to worry about any "compatibility" issues here.

thanks,

greg k-h