When checking an extended attrbiute block for correctness, we check if
the starting offset plus the value size exceeds the end of the block.
However, we weren't checking if the size was too large, and if it is
so large that it triggers a wraparound when we added the starting
offset, we won't notice the problem. Add the missing check.
Reported-by: Nils Bars <[email protected]>
Reported-by: Moritz Schlögel <[email protected]>
Reported-by: Nico Schiller <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
e2fsck/pass1.c | 5 +++--
lib/ext2fs/ext2_ext_attr.h | 11 +++++++++++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index 2a17bb8a..11d7ce93 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -2556,8 +2556,9 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
break;
}
if (entry->e_value_inum == 0) {
- if (entry->e_value_offs + entry->e_value_size >
- fs->blocksize) {
+ if (entry->e_value_size > EXT2_XATTR_SIZE_MAX ||
+ (entry->e_value_offs + entry->e_value_size >
+ fs->blocksize)) {
if (fix_problem(ctx, PR_1_EA_BAD_VALUE, pctx))
goto clear_extattr;
break;
diff --git a/lib/ext2fs/ext2_ext_attr.h b/lib/ext2fs/ext2_ext_attr.h
index f2042ed5..c6068c48 100644
--- a/lib/ext2fs/ext2_ext_attr.h
+++ b/lib/ext2fs/ext2_ext_attr.h
@@ -57,6 +57,17 @@ struct ext2_ext_attr_entry {
#define EXT2_XATTR_SIZE(size) \
(((size) + EXT2_EXT_ATTR_ROUND) & ~EXT2_EXT_ATTR_ROUND)
+/*
+ * XATTR_SIZE_MAX is currently 64k, but for the purposes of checking
+ * for file system consistency errors, we use a somewhat bigger value.
+ * This allows XATTR_SIZE_MAX to grow in the future, but by using this
+ * instead of INT_MAX for certain consistency checks, we don't need to
+ * worry about arithmetic overflows. (Actually XATTR_SIZE_MAX is
+ * defined in include/uapi/linux/limits.h, so changing it is going
+ * not going to be trivial....)
+ */
+#define EXT2_XATTR_SIZE_MAX (1 << 24)
+
#ifdef __KERNEL__
# ifdef CONFIG_EXT2_FS_EXT_ATTR
extern int ext2_get_ext_attr(struct inode *, const char *, char *, size_t, int);
--
2.31.0
On Tue, Jun 07, 2022 at 12:24:41AM -0400, Theodore Ts'o wrote:
> When checking an extended attrbiute block for correctness, we check if
> the starting offset plus the value size exceeds the end of the block.
> However, we weren't checking if the size was too large, and if it is
> so large that it triggers a wraparound when we added the starting
> offset, we won't notice the problem. Add the missing check.
Looks good.
Reviewed-by: Lukas Czerner <[email protected]>
>
> Reported-by: Nils Bars <[email protected]>
> Reported-by: Moritz Schl?gel <[email protected]>
> Reported-by: Nico Schiller <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
> ---
> e2fsck/pass1.c | 5 +++--
> lib/ext2fs/ext2_ext_attr.h | 11 +++++++++++
> 2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 2a17bb8a..11d7ce93 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -2556,8 +2556,9 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
> break;
> }
> if (entry->e_value_inum == 0) {
> - if (entry->e_value_offs + entry->e_value_size >
> - fs->blocksize) {
> + if (entry->e_value_size > EXT2_XATTR_SIZE_MAX ||
> + (entry->e_value_offs + entry->e_value_size >
> + fs->blocksize)) {
> if (fix_problem(ctx, PR_1_EA_BAD_VALUE, pctx))
> goto clear_extattr;
> break;
> diff --git a/lib/ext2fs/ext2_ext_attr.h b/lib/ext2fs/ext2_ext_attr.h
> index f2042ed5..c6068c48 100644
> --- a/lib/ext2fs/ext2_ext_attr.h
> +++ b/lib/ext2fs/ext2_ext_attr.h
> @@ -57,6 +57,17 @@ struct ext2_ext_attr_entry {
> #define EXT2_XATTR_SIZE(size) \
> (((size) + EXT2_EXT_ATTR_ROUND) & ~EXT2_EXT_ATTR_ROUND)
>
> +/*
> + * XATTR_SIZE_MAX is currently 64k, but for the purposes of checking
> + * for file system consistency errors, we use a somewhat bigger value.
> + * This allows XATTR_SIZE_MAX to grow in the future, but by using this
> + * instead of INT_MAX for certain consistency checks, we don't need to
> + * worry about arithmetic overflows. (Actually XATTR_SIZE_MAX is
> + * defined in include/uapi/linux/limits.h, so changing it is going
> + * not going to be trivial....)
> + */
> +#define EXT2_XATTR_SIZE_MAX (1 << 24)
> +
> #ifdef __KERNEL__
> # ifdef CONFIG_EXT2_FS_EXT_ATTR
> extern int ext2_get_ext_attr(struct inode *, const char *, char *, size_t, int);
> --
> 2.31.0
>