linux-kernel - Re: [PATCH] xfs: mark sb

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a1n3ME0f_mVTcph8aQ3x7rFriypd0kzxz9Up+sZJdLb=g@mail.gmail.com>
Date:   Fri, 25 May 2018 22:16:12 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Eric Sandeen <sandeen@...hat.com>
Cc:     "Darrick J. Wong" <darrick.wong@...cle.com>,
        linux-xfs <linux-xfs@...r.kernel.org>,
        Martin Sebor <msebor@...il.com>,
        Brian Foster <bfoster@...hat.com>,
        Dave Chinner <dchinner@...hat.com>,
        Eric Sandeen <sandeen@...deen.net>,
        Dan Williams <dan.j.williams@...el.com>,
        Ross Zwisler <ross.zwisler@...ux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] xfs: mark sb_fname as nonstring

On Fri, May 25, 2018 at 6:53 PM, Eric Sandeen <sandeen@...hat.com> wrote:
> On 5/25/18 10:14 AM, Arnd Bergmann wrote:
>>
>> gcc-8 reports two warnings for the newly added getlabel/setlabel code:
>
>
> Thanks for catching these.
>
> The patch summary confuses me, what does "mark sb_fname as nonstring"
> mean in the context of the actual patch?

My mistake. I tried a few different approaches and ended up using the
subject line from an earlier version with a later patch.

The 'nonstring' annotation is a variable attribute that gets gcc-8
to shut up about -Wstringop-truncation warnings, and is intended
to mark those character arrays that are not expected to be
null-terminated but still used with strncpy().

>>         spin_unlock(&mp->m_sb_lock);
>>         /* xfs on-disk label is 12 chars, be sure we send a null to user
>> */
>>         label[XFSLABEL_MAX] = '\0';
>> -       if (copy_to_user(user_label, label, sizeof(sbp->sb_fname)))
>> +       if (copy_to_user(user_label, label, sizeof(label)))
>
>
>
> ok.  (odd how this is ok for copy_to_user but not for strncpy above) :)

No idea. Maybe the gcc bug only happens with struct members but
not local variables?

>>                 return -EFAULT;
>>         return 0;
>>   }
>> @@ -1860,7 +1860,7 @@ xfs_ioc_setlabel(
>>         spin_lock(&mp->m_sb_lock);
>>         memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
>> -       strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
>> +       memcpy(sbp->sb_fname, label, len);
>
>
> Hm but len = strnlen(label, XFSLABEL_MAX + 1);
> which could be one longer than sbp->sb_fname, no?

We have an explicit check for that, so I think it's ok:

        if (len > sizeof(sbp->sb_fname))
                return -EINVAL;

       Arnd