drivers/md/dm-cache-metadata.c:1705:9: warning: 'strncpy' specified bound 16 equals destination size

drivers/md/dm-cache-metadata.c:1705:9: warning: 'strncpy' specified bound 16 equals destination size

[kernel test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head:   633b47cb009d09dc8f4ba9cdb3a0ca138809c7c7
commit: 23066c3f4e2146da8c7d1505729f4409f4d93d28 Compiler Attributes: enable -Wstringop-truncation on W=1 (gcc >= 8)
date:   5 years ago
config: arm-randconfig-002-20230928 (https://download.01.org/0day-ci/archive/20230928/202309280610.sLGSPkOG-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230928/202309280610.sLGSPkOG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309280610.sLGSPkOG-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In function 'write_hints',
       inlined from 'dm_cache_write_hints' at drivers/md/dm-cache-metadata.c:1729:6:
>> drivers/md/dm-cache-metadata.c:1705:9: warning: 'strncpy' specified bound 16 equals destination size [-Wstringop-truncation]
    1705 |         strncpy(cmd->policy_name, policy_name, sizeof(cmd->policy_name));
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/strncpy +1705 drivers/md/dm-cache-metadata.c

4e781b498ee500 Joe Thornber 2016-09-15  1689  
4e781b498ee500 Joe Thornber 2016-09-15  1690  /*
4e781b498ee500 Joe Thornber 2016-09-15  1691   * It's quicker to always delete the hint array, and recreate with
4e781b498ee500 Joe Thornber 2016-09-15  1692   * dm_array_new().
4e781b498ee500 Joe Thornber 2016-09-15  1693   */
4e781b498ee500 Joe Thornber 2016-09-15  1694  static int write_hints(struct dm_cache_metadata *cmd, struct dm_cache_policy *policy)
c6b4fcbad044e6 Joe Thornber 2013-03-01  1695  {
c6b4fcbad044e6 Joe Thornber 2013-03-01  1696  	int r;
c6b4fcbad044e6 Joe Thornber 2013-03-01  1697  	size_t hint_size;
c6b4fcbad044e6 Joe Thornber 2013-03-01  1698  	const char *policy_name = dm_cache_policy_get_name(policy);
4e7f506f642963 Mike Snitzer 2013-03-20  1699  	const unsigned *policy_version = dm_cache_policy_get_version(policy);
c6b4fcbad044e6 Joe Thornber 2013-03-01  1700  
c6b4fcbad044e6 Joe Thornber 2013-03-01  1701  	if (!policy_name[0] ||
c6b4fcbad044e6 Joe Thornber 2013-03-01  1702  	    (strlen(policy_name) > sizeof(cmd->policy_name) - 1))
c6b4fcbad044e6 Joe Thornber 2013-03-01  1703  		return -EINVAL;
c6b4fcbad044e6 Joe Thornber 2013-03-01  1704  
c6b4fcbad044e6 Joe Thornber 2013-03-01 @1705  	strncpy(cmd->policy_name, policy_name, sizeof(cmd->policy_name));
4e7f506f642963 Mike Snitzer 2013-03-20  1706  	memcpy(cmd->policy_version, policy_version, sizeof(cmd->policy_version));
c6b4fcbad044e6 Joe Thornber 2013-03-01  1707  
c6b4fcbad044e6 Joe Thornber 2013-03-01  1708  	hint_size = dm_cache_policy_get_hint_size(policy);
c6b4fcbad044e6 Joe Thornber 2013-03-01  1709  	if (!hint_size)
c6b4fcbad044e6 Joe Thornber 2013-03-01  1710  		return 0; /* short-circuit hints initialization */
c6b4fcbad044e6 Joe Thornber 2013-03-01  1711  	cmd->policy_hint_size = hint_size;
c6b4fcbad044e6 Joe Thornber 2013-03-01  1712  
c6b4fcbad044e6 Joe Thornber 2013-03-01  1713  	if (cmd->hint_root) {
c6b4fcbad044e6 Joe Thornber 2013-03-01  1714  		r = dm_array_del(&cmd->hint_info, cmd->hint_root);
c6b4fcbad044e6 Joe Thornber 2013-03-01  1715  		if (r)
c6b4fcbad044e6 Joe Thornber 2013-03-01  1716  			return r;
c6b4fcbad044e6 Joe Thornber 2013-03-01  1717  	}
c6b4fcbad044e6 Joe Thornber 2013-03-01  1718  
4e781b498ee500 Joe Thornber 2016-09-15  1719  	return dm_array_new(&cmd->hint_info, &cmd->hint_root,
c6b4fcbad044e6 Joe Thornber 2013-03-01  1720  			    from_cblock(cmd->cache_blocks),
4e781b498ee500 Joe Thornber 2016-09-15  1721  			    get_hint, policy);
0596661f0a16d9 Joe Thornber 2014-04-03  1722  }
0596661f0a16d9 Joe Thornber 2014-04-03  1723  

:::::: The code at line 1705 was first introduced by commit
:::::: c6b4fcbad044e6fffcc75bba160e720eb8d67d17 dm: add cache target

:::::: TO: Joe Thornber <ejt@redhat.com>
:::::: CC: Alasdair G Kergon <agk@redhat.com>

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: drivers/md/dm-cache-metadata.c:1705:9: warning: 'strncpy' specified bound 16 equals destination size

[Miguel Ojeda]

On Thu, Sep 28, 2023 at 12:24 AM kernel test robot <lkp@intel.com> wrote:
>
>    In function 'write_hints',
>        inlined from 'dm_cache_write_hints' at drivers/md/dm-cache-metadata.c:1729:6:
> >> drivers/md/dm-cache-metadata.c:1705:9: warning: 'strncpy' specified bound 16 equals destination size [-Wstringop-truncation]
>     1705 |         strncpy(cmd->policy_name, policy_name, sizeof(cmd->policy_name));
>          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This looks fine given the check above it. In any case, since it
appears the code wants to terminate the string, this should use
`strscpy*()`, right?

Cheers,
Miguel

Re: drivers/md/dm-cache-metadata.c:1705:9: warning: 'strncpy' specified bound 16 equals destination size

[Kees Cook]

On Thu, Sep 28, 2023 at 11:36:46AM +0200, Miguel Ojeda wrote:
> On Thu, Sep 28, 2023 at 12:24 AM kernel test robot <lkp@intel.com> wrote:
> >
> >    In function 'write_hints',
> >        inlined from 'dm_cache_write_hints' at drivers/md/dm-cache-metadata.c:1729:6:
> > >> drivers/md/dm-cache-metadata.c:1705:9: warning: 'strncpy' specified bound 16 equals destination size [-Wstringop-truncation]
> >     1705 |         strncpy(cmd->policy_name, policy_name, sizeof(cmd->policy_name));
> >          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This looks fine given the check above it. In any case, since it
> appears the code wants to terminate the string, this should use
> `strscpy*()`, right?

I agree. I can't tell if _pad is needed though. I think struct
dm_cache_metadata is only used internally? And I see at least the
initial allocation is zeroed:

        cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);

Regardless, for background,

struct has:
        char policy_name[CACHE_POLICY_NAME_SIZE];

code does:
        const char *policy_name = dm_cache_policy_get_name(policy);

        if (!policy_name[0] ||
            (strlen(policy_name) > sizeof(cmd->policy_name) - 1))
                return -EINVAL;

        strncpy(cmd->policy_name, policy_name, sizeof(cmd->policy_name));


Justin, can you add this to your queue (if it's not already there)?

-Kees

-- 
Kees Cook

Re: drivers/md/dm-cache-metadata.c:1705:9: warning: 'strncpy' specified bound 16 equals destination size

[Justin Stitt]

On Sat, Sep 30, 2023 at 2:49 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Sep 28, 2023 at 11:36:46AM +0200, Miguel Ojeda wrote:
> > On Thu, Sep 28, 2023 at 12:24 AM kernel test robot <lkp@intel.com> wrote:
> > >
> > >    In function 'write_hints',
> > >        inlined from 'dm_cache_write_hints' at drivers/md/dm-cache-metadata.c:1729:6:
> > > >> drivers/md/dm-cache-metadata.c:1705:9: warning: 'strncpy' specified bound 16 equals destination size [-Wstringop-truncation]
> > >     1705 |         strncpy(cmd->policy_name, policy_name, sizeof(cmd->policy_name));
> > >          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > This looks fine given the check above it. In any case, since it
> > appears the code wants to terminate the string, this should use
> > `strscpy*()`, right?
>
> I agree. I can't tell if _pad is needed though. I think struct
> dm_cache_metadata is only used internally? And I see at least the
> initial allocation is zeroed:
>
>         cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>
> Regardless, for background,
>
> struct has:
>         char policy_name[CACHE_POLICY_NAME_SIZE];
>
> code does:
>         const char *policy_name = dm_cache_policy_get_name(policy);
>
>         if (!policy_name[0] ||
>             (strlen(policy_name) > sizeof(cmd->policy_name) - 1))
>                 return -EINVAL;
>
>         strncpy(cmd->policy_name, policy_name, sizeof(cmd->policy_name));
>
>
> Justin, can you add this to your queue (if it's not already there)?

I sent a patch out earlier this week [1].

The line # on the diff v.s the warning is off though. I suspect I may
need a rebase if it doesn't apply clean.

>
> -Kees
>
> --
> Kees Cook

[1]: https://lore.kernel.org/all/20230925-strncpy-drivers-md-dm-cache-metadata-c-v1-1-4b75c7db0cfe@google.com/