On Fri, Mar 30, 2018 at 12:32:12PM -0700, Eric Biggers wrote:
> But, if we're actually operating under the assumption that someone can modify
> the xattr block's buffer_head concurrently, we'd actually need to copy it to a
> temporary buffer and validate it there, rather than reading directly from the
> cache. Likewise for the other ext4 metadata such as directory blocks. Checking
> one specific field in one specific place is not nearly enough. And if we did
> read directly from the cache we'd have to use volatile memory accesses.
We don't need to copy *all* of the the block and validate it *all*.
We just need to validate the bits which might cause a buffer overrun.
For example:
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index b0a9ba8e576a..7b7e40a05419 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -547,8 +547,14 @@ ext4_xattr_block_get(struct inode *inode, int name_index, const char *name,
if (error)
goto cleanup;
} else {
- memcpy(buffer, bh->b_data +
- le16_to_cpu(entry->e_value_offs), size);
+ __u16 offset = le16_to_cpu(entry->e_value_offs);
+ unsigned long blocksize = inode->i_sb->s_s_blocksize;
+
+ error = -ERANGE;
+ if (unlikely((offset >= blocksize) ||
+ ((offset + size) >= blocksize)))
+ goto cleanup;
+ memcpy(buffer, bh->b_data + offset, size);
}
}
error = size;
See? This is much more efficient, doesn't require that we do a memory
allocation for the temp buffer (which might fail), doesn't require any
locking (which would be the other way to try to solve the problem), etc.
- Ted
P.S. We should change all of the return -ERANGE to -EFSCORRUPTED and
arrange to have ext4_error() getting called. Another cleanup patch.
The fs/ext4/xattr.c file is ripe for a lot of cleanup /
robustification patches...
