Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 2 Jun 2021 13:52:59 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Jay Vosburgh <jay.vosburgh@canonical.com>
Cc:     kernel test robot <lkp@intel.com>,
        Veaceslav Falico <vfalico@gmail.com>,
        Andy Gospodarek <andy@greyhouse.net>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2] net: bonding: Use strscpy() instead of
 manually-truncated strncpy()
Message-ID: <202106021352.484D660@keescook>
References: <20210602203138.4082470-1-keescook@chromium.org>
 <7214.1622666840@famine>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7214.1622666840@famine>
Precedence: bulk

On Wed, Jun 02, 2021 at 01:47:20PM -0700, Jay Vosburgh wrote:
> Kees Cook <keescook@chromium.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@intel.com>
> >Link: https://lore.kernel.org/lkml/202102150705.fdR6obB0-lkp@intel.com
> >Signed-off-by: Kees Cook <keescook@chromium.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@canonical.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@canonical.com

-- 
Kees Cook