linux-hardening - Re: [PATCH v2] net: bonding: Use strscpy() instead of manually-truncated strncpy()

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] [day] [month] [year] [list]

Message-ID: <202106021352.484D660@keescook>
Date:   Wed, 2 Jun 2021 13:52:59 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Jay Vosburgh <jay.vosburgh@...onical.com>
Cc:     kernel test robot <lkp@...el.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] net: bonding: Use strscpy() instead of
 manually-truncated strncpy()

On Wed, Jun 02, 2021 at 01:47:20PM -0700, Jay Vosburgh wrote:
> Kees Cook <keescook@...omium.org> wrote:
> 
> >Silence this warning by just using strscpy_pad() directly:
> >
> >>> drivers/net/bonding/bond_main.c:4877:3: warning: 'strncpy' specified bound 16 equals destination size [-Wstringop-truncation]
> >    4877 |   strncpy(params->primary, primary, IFNAMSIZ);
> >         |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> >Additionally replace other strncpy() uses, as it is considered deprecated:
> >https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> >
> >Reported-by: kernel test robot <lkp@...el.com>
> >Link: https://lore.kernel.org/lkml/202102150705.fdR6obB0-lkp@intel.com
> >Signed-off-by: Kees Cook <keescook@...omium.org>
> 
> 	There's one more "strncpy(...); primary[IFNAMSIZ - 1] = 0;" set
> in bond_options.c:bond_option_primary_set(), doesn't it also generate
> this warning?

Gah, I was looking only in the same file not the whole directory. :)

v3 on the way!

> 
> 	Either way, the change looks good to me.

Thanks!

> 
> Acked-by: Jay Vosburgh <jay.vosburgh@...onical.com>
> 
> 	-J
> 
> 
> >---
> >v2:
> > - switch to strscpy_pad() and replace earlier strncpy() too
> >v1: https://lore.kernel.org/lkml/20210602181133.3326856-1-keescook@chromium.org
> >---
> > drivers/net/bonding/bond_main.c | 8 +++-----
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index c5a646d06102..e9cb716ad849 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -620,7 +620,7 @@ static int bond_check_dev_link(struct bonding *bond,
> > 		 */
> > 
> > 		/* Yes, the mii is overlaid on the ifreq.ifr_ifru */
> >-		strncpy(ifr.ifr_name, slave_dev->name, IFNAMSIZ);
> >+		strscpy_pad(ifr.ifr_name, slave_dev->name, IFNAMSIZ);
> > 		mii = if_mii(&ifr);
> > 		if (ioctl(slave_dev, &ifr, SIOCGMIIPHY) == 0) {
> > 			mii->reg_num = MII_BMSR;
> >@@ -5329,10 +5329,8 @@ static int bond_check_params(struct bond_params *params)
> > 			(struct reciprocal_value) { 0 };
> > 	}
> > 
> >-	if (primary) {
> >-		strncpy(params->primary, primary, IFNAMSIZ);
> >-		params->primary[IFNAMSIZ - 1] = 0;
> >-	}
> >+	if (primary)
> >+		strscpy_pad(params->primary, primary, sizeof(params->primary));
> > 
> > 	memcpy(params->arp_targets, arp_target, sizeof(arp_target));
> > 
> >-- 
> >2.25.1
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@...onical.com

-- 
Kees Cook