On 2019/06/06 17:05, Nikolay Borisov wrote:
>
>
> On 6.06.19 г. 10:49 ч., Naohiro Aota wrote:
>> xattr value is not NULL-terminated string. When you specify "lz" as the
>> property value, strncmp("lzo", value, 3) will try to read one byte after
>> the value buffer, causing the following OOB access. Fix this out-of-bound
>> by explicitly check the required length.
>>
>>(snip)
>>
>> Fixes: 272e5326c783 ("btrfs: prop: fix vanished compression property after failed set")
>> Fixes: 50398fde997f ("btrfs: prop: fix zstd compression parameter validation")
>> Cc: stable@xxxxxxxxxxxxxxx # 4.14+: 802a5c69584a: btrfs: prop: use common helper for type to string conversion
>> Cc: stable@xxxxxxxxxxxxxxx # 4.14+: 3dcf96c7b9fe: btrfs: drop redundant forward declaration in props.c
>> Cc: stable@xxxxxxxxxxxxxxx # 4.14+
>> Signed-off-by: Naohiro Aota <naohiro.aota@xxxxxxx>
>
> We caught that one yesterday and were testing various fixes for it
> Johannes just sent his version which IMO makes the code a bit more
> maintainable.
>
yeah, that looks good to me. It's much more easy to add new compression
type. Please pick that one.
>
>> ---
>> fs/btrfs/props.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>> index a9e2e66152ee..428141bf545d 100644
>> --- a/fs/btrfs/props.c
>> +++ b/fs/btrfs/props.c
>> @@ -257,11 +257,11 @@ static int prop_compression_validate(const char *value, size_t len)
>> if (!value)
>> return 0;
>>
>> - if (!strncmp("lzo", value, 3))
>> + if (len >= 3 && !strncmp("lzo", value, 3))
>> return 0;
>> - else if (!strncmp("zlib", value, 4))
>> + else if (len >= 4 && !strncmp("zlib", value, 4))
>> return 0;
>> - else if (!strncmp("zstd", value, 4))
>> + else if (len >= 4 && !strncmp("zstd", value, 4))
>> return 0;
>>
>> return -EINVAL;
>> @@ -281,12 +281,12 @@ static int prop_compression_apply(struct inode *inode, const char *value,
>> return 0;
>> }
>>
>> - if (!strncmp("lzo", value, 3)) {
>> + if (len >= 3 && !strncmp("lzo", value, 3)) {
>> type = BTRFS_COMPRESS_LZO;
>> btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
>> - } else if (!strncmp("zlib", value, 4)) {
>> + } else if (len >= 4 && !strncmp("zlib", value, 4)) {
>> type = BTRFS_COMPRESS_ZLIB;
>> - } else if (!strncmp("zstd", value, 4)) {
>> + } else if (len >= 4 && !strncmp("zstd", value, 4)) {
>> type = BTRFS_COMPRESS_ZSTD;
>> btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
>> } else {
>
> This seems redundant as ->validates is supposed to always be called
> before calling ->apply.
>
Indeed.
