All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Theodore Ts'o" <tytso@mit.edu>
To: Deepanshu Kartikey <kartikey406@gmail.com>
Cc: adilger.kernel@dilger.ca, linux-ext4@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	syzbot+4c9d23743a2409b80293@syzkaller.appspotmail.com
Subject: Re: [PATCH] ext4: validate xattr entries in ext4_xattr_move_to_block
Date: Tue, 23 Sep 2025 08:09:54 -0400	[thread overview]
Message-ID: <20250923120954.GA531144@mit.edu> (raw)
In-Reply-To: <20250923092512.1088241-1-kartikey406@gmail.com>

On Tue, Sep 23, 2025 at 02:55:12PM +0530, Deepanshu Kartikey wrote:
> During inode expansion, ext4_xattr_move_to_block() processes xattr entries
> from on-disk structures without validating their integrity. Corrupted
> filesystems may contain xattr entries where e_value_size is zero but
> e_value_inum is non-zero, indicating the entry claims to store its value
> in a separate inode but has no actual value.
> 
> This corruption pattern leads to a WARNING in ext4_xattr_block_set() when
> it encounters i->value_len of zero while i->in_inode is set, violating
> the function's invariant that in-inode xattrs must have non-zero length.
> 
> Add validation in ext4_xattr_move_to_block() to detect this specific
> corruption pattern and return -EFSCORRUPTED, preventing the invalid
> data from propagating to downstream functions and causing warnings.
> 
> Reported-by: syzbot+4c9d23743a2409b80293@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=4c9d23743a2409b80293
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>

Thanks for the patch!  Could you try moving the validation test to the
check_xattrs() function?  This should hopefully catch other
maliciously fuzzed file systems so it might address other syzbot
complaints.

Something like:

		if (ea_ino && !size) {
			err_str = "invalid size in ea xattr";
			goto errout;
		}

In retrospect, we probably should have had the code interpret
e_value_size==0 as meaning that the xattr entry is always unused, so
that tests such as:

		if (!last->e_value_inum && last->e_value_size) {

could become

		if (last->e_value_size) {

But there are also places where the code assumes that if e_value_inum
is non-zero, it doesn't need to test e_value_size.

It should be the case where whenever we clear e_value_inum, we also
set i_value_size to zero.  So having e_value_num!=0 && e_value_size==0
should only be the case when someone is trying to maliciously play
games with us.

Cheers,

						- Ted

      reply	other threads:[~2025-09-23 12:10 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-09-23  9:25 [PATCH] ext4: validate xattr entries in ext4_xattr_move_to_block Deepanshu Kartikey
2025-09-23 12:09 ` Theodore Ts'o [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250923120954.GA531144@mit.edu \
    --to=tytso@mit.edu \
    --cc=adilger.kernel@dilger.ca \
    --cc=kartikey406@gmail.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=syzbot+4c9d23743a2409b80293@syzkaller.appspotmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.