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@vger.kernel.org # 4.14+: 802a5c69584a: btrfs: prop: use common helper for type to string conversion Cc: stable@vger.kernel.org # 4.14+: 3dcf96c7b9fe: btrfs: drop redundant forward declaration in props.c Cc: stable@vger.kernel.org # 4.14+ Signed-off-by: Naohiro Aota naohiro.aota@wdc.com
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.