linux-kernel - 回复: [PATCH] btrfs: replace snprintf in show functions with sysfs

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

Message-ID: <SL2PR06MB3082B71AFE2C42CABDD5E6D6BDB79@SL2PR06MB3082.apcprd06.prod.outlook.com>
Date:   Wed, 13 Oct 2021 11:01:27 +0000
From:   王擎 <wangqing@...o.com>
To:     Qu Wenruo <quwenruo.btrfs@....com>,
        "dsterba@...e.cz" <dsterba@...e.cz>,
        Anand Jain <anand.jain@...cle.com>, Chris Mason <clm@...com>,
        Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>,
        "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: 回复: [PATCH] btrfs: replace snprintf in show functions with sysfs_emit


>> On Wed, Oct 13, 2021 at 03:51:33PM +0800, Anand Jain wrote:
>>> On 13/10/2021 11:28, Qing Wang wrote:
>>>> coccicheck complains about the use of snprintf() in sysfs show functions.
>>>
>>> It looks like the reason is snprintf() unaware of the PAGE_SIZE
>>> max_limit of the buf.
>>>
>>>> Fix the following coccicheck warning:
>>>> fs/btrfs/sysfs.c:335:8-16: WARNING: use scnprintf or sprintf.
>
>IIRC sprintf() is less safe than snprintf().
>Is the check really correct to mention sprintf()?

device_attr_show.cocci metions show() must not use snprintf() 
when formatting the value to be returned to user space.
If you can guarantee that an overflow will never happen you
can use sprintf() otherwise you must use scnprintf().

My understanding is this is not only to solve the possible 
overflow issue, snprintf() returns the length of the string, not 
the length actually written. We can directly use sysfs_emit() here.

Thanks,

Qing

>>>
>>> Hm. We use snprintf() at quite a lot more places in sysfs.c and, I don't
>>> see them getting this fix. Why?
>>
>> I guess the patch is only addressing the warning for snprintf, reading
>> the sources would show how many more conversions could have been done of
>> scnprintf calls.
>>
>>>> Use sysfs_emit instead of scnprintf or sprintf makes more sense.
>>>
>>> Below commit has added it. Nice.
>>>
>>> commit 2efc459d06f1630001e3984854848a5647086232
>>> Date:   Wed Sep 16 13:40:38 2020 -0700
>>>
>>>       sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs out
>>
>> The conversion to the standard helper is good, but should be done
>> in the entire file.
>>
>
> Yeah, the same idea, all sysfs interface should convert to the new
> interface, not only the snprintf().
>
> Thanks,
> Qu