On Fri 24-05-19 14:11:34, cgxu519@xxxxxxxxxxx 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@xxxxxxxxxxx> > > > > 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 -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
