| Message ID | 20210602203138.4082470-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: bonding: Use strscpy() instead of manually-truncated strncpy() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 5 this patch: 5 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 5 this patch: 5 |
| netdev/header_inline | success | Link |
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? Either way, the change looks good to me. 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
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
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));