| Message ID | 20190522082846.22296-1-cgxu519@zoho.com.cn |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ext2: strengthen value length check in ext2_xattr_set() | expand |
On Wed 22-05-19 16:28:46, Chengguang Xu wrote: > Actually maximum length of a valid entry value is not > ->s_blocksize because header, last entry and entry > name will also occupy some spaces. This patch > strengthens the value length check and return -ERANGE > when the length is larger than allowed maximum length. > > Signed-off-by: Chengguang Xu <cgxu519@zoho.com.cn> Thanks for the patch! But what's the point of this change? We would return ERANGE instead of ENOSPC? I don't think that's serious enough to warrant changing existing behavior... > @@ -423,7 +423,10 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name, > if (name == NULL) > return -EINVAL; > name_len = strlen(name); > - if (name_len > 255 || value_len > sb->s_blocksize) > + max_len = sb->s_blocksize - sizeof(struct ext2_xattr_header) > + - sizeof(__u32); > + if (name_len > 255 || > + EXT2_XATTR_LEN(name_len) + EXT2_XATTR_SIZE(value_len) > max_len) > return -ERANGE; > down_write(&EXT2_I(inode)->xattr_sem); > if (EXT2_I(inode)->i_file_acl) { Honza
On Wed, 2019-05-22 at 11:50 +0200, Jan Kara wrote: > On Wed 22-05-19 16:28:46, Chengguang Xu wrote: > > Actually maximum length of a valid entry value is not > > ->s_blocksize because header, last entry and entry > > name will also occupy some spaces. This patch > > strengthens the value length check and return -ERANGE > > when the length is larger than allowed maximum length. > > > > Signed-off-by: Chengguang Xu <cgxu519@zoho.com.cn> > > Thanks for the patch! But what's the point of this change? We would return > ERANGE instead of ENOSPC? I don't think that's serious enough to warrant > changing existing behavior... Hi Jan, Thanks for the review. The motivation is seprating error situations of ENOSPC/ERANGE because ENOSPC is giving a hint that we can save an EA entry (name+value > allowed maximum length) by deleting some existing entries. However, as you has pointed out, I also think the difference is not so important because some EA entries (like security index) is invisible for user... Thanks, Chengguang > > > @@ -423,7 +423,10 @@ ext2_xattr_set(struct inode *inode, int name_index, > > const char *name, > > if (name == NULL) > > return -EINVAL; > > name_len = strlen(name); > > - if (name_len > 255 || value_len > sb->s_blocksize) > > + max_len = sb->s_blocksize - sizeof(struct ext2_xattr_header) > > + - sizeof(__u32); > > + if (name_len > 255 || > > + EXT2_XATTR_LEN(name_len) + EXT2_XATTR_SIZE(value_len) > max_len) > > return -ERANGE; > > down_write(&EXT2_I(inode)->xattr_sem); > > if (EXT2_I(inode)->i_file_acl) { > > Honza >
On Wed, 2019-05-22 at 11:50 +0200, Jan Kara wrote: > On Wed 22-05-19 16:28:46, Chengguang Xu wrote: > > Actually maximum length of a valid entry value is not > > ->s_blocksize because header, last entry and entry > > name will also occupy some spaces. This patch > > strengthens the value length check and return -ERANGE > > when the length is larger than allowed maximum length. > > > > Signed-off-by: Chengguang Xu <cgxu519@zoho.com.cn> > > Thanks for the patch! But what's the point of this change? We would return > ERANGE instead of ENOSPC? I don't think that's serious enough to warrant > changing existing behavior... Hi Jan, Instead of adding the check here, I propose to change value size limit check in ext2_xattr_entry_valid(). size = le32_to_cpu(entry->e_value_size); if (size > end_offs || le16_to_cpu(entry->e_value_offs) + size > end_offs) Change to size = EXT2_XATTR_SIZE(le32_to_cpu(entry->e_value_size)); if (size >= end_offs - sizeof(struct ext2_xattr_header) - sizeof(__u32) || le16_to_cpu(entry->e_value_offs) + size > end_offs) Will you agree this change? Thanks, Chengguang
On Fri 24-05-19 14:11:34, cgxu519@zoho.com.cn wrote: > On Wed, 2019-05-22 at 11:50 +0200, Jan Kara wrote: > > On Wed 22-05-19 16:28:46, Chengguang Xu wrote: > > > Actually maximum length of a valid entry value is not > > > ->s_blocksize because header, last entry and entry > > > name will also occupy some spaces. This patch > > > strengthens the value length check and return -ERANGE > > > when the length is larger than allowed maximum length. > > > > > > Signed-off-by: Chengguang Xu <cgxu519@zoho.com.cn> > > > > Thanks for the patch! But what's the point of this change? We would return > > ERANGE instead of ENOSPC? I don't think that's serious enough to warrant > > changing existing behavior... > > Hi Jan, > > Instead of adding the check here, I propose to change value > size limit check in ext2_xattr_entry_valid(). > > size = le32_to_cpu(entry->e_value_size); > if (size > end_offs || > le16_to_cpu(entry->e_value_offs) + size > end_offs) > > Change to > > size = EXT2_XATTR_SIZE(le32_to_cpu(entry->e_value_size)); > if (size >= end_offs - sizeof(struct ext2_xattr_header) - sizeof(__u32) || > le16_to_cpu(entry->e_value_offs) + size > end_offs) I don't think this makes a big difference. Look: end_offs is always aligned to EXT2_XATTR_PAD (it is always block size) so if entry->e_value_offs is properly aligned (which we may want to check), then le16_to_cpu(entry->e_value_offs) + EXT2_XATTR_SIZE(size) > end_offs if and only if le16_to_cpu(entry->e_value_offs) + size > end_offs. Also the check le16_to_cpu(entry->e_value_offs) + size > end_offs is the essential and strongest part - it checks whether the value does not extend beyond block. The check size > end_offs is needed only for the case where le16_to_cpu(entry->e_value_offs) + size would overflow and result in a negative number. Honza
diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c index f1f857b83b45..425c8e29d3cb 100644 --- a/fs/ext2/xattr.c +++ b/fs/ext2/xattr.c @@ -399,7 +399,7 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name, struct buffer_head *bh = NULL; struct ext2_xattr_header *header = NULL; struct ext2_xattr_entry *here, *last; - size_t name_len, free, min_offs = sb->s_blocksize; + size_t name_len, free, min_offs = sb->s_blocksize, max_len; int not_found = 1, error; char *end; @@ -423,7 +423,10 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name, if (name == NULL) return -EINVAL; name_len = strlen(name); - if (name_len > 255 || value_len > sb->s_blocksize) + max_len = sb->s_blocksize - sizeof(struct ext2_xattr_header) + - sizeof(__u32); + if (name_len > 255 || + EXT2_XATTR_LEN(name_len) + EXT2_XATTR_SIZE(value_len) > max_len) return -ERANGE; down_write(&EXT2_I(inode)->xattr_sem); if (EXT2_I(inode)->i_file_acl) {
Actually maximum length of a valid entry value is not ->s_blocksize because header, last entry and entry name will also occupy some spaces. This patch strengthens the value length check and return -ERANGE when the length is larger than allowed maximum length. Signed-off-by: Chengguang Xu <cgxu519@zoho.com.cn> --- fs/ext2/xattr.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)