Re: [PATCH v4] ubifs: zero stale data beyond EOF before file size grows

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ubifs: stale mmap data exposed on short copy in ubifs_write_iter()
--

commit afed492b9caec08994a0a97a427ac39e28d77180
Author: Nan Lin <lin.nan@h3c.com>

ubifs: zero stale data beyond EOF before file size grows

This commit adds ubifs_zero_eof_hole() to zero stale mmap data beyond EOF in
the old-EOF folio during do_setattr() and ubifs_write_iter(), and restructures
ubifs_write_iter() to serialize writes under inode_lock().

> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index e73c28b12f97f..6f2c83b6f4333 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -202,6 +202,45 @@ static void release_existing_page_budget(struct ubifs_info *c)
>  	ubifs_release_budget(c, &req);
>  }
>  
> +/**
> + * ubifs_zero_eof_hole - zero the stale tail of the old-EOF folio.
> + * @inode: inode whose size is about to grow
> + * @new_end: new end of the file, only used to bound the tail
> + *
> + * Zero the cached folio containing the old EOF for [old EOF,
> + * min(folio end, @new_end)) so a size extension does not expose stale
> + * data left beyond EOF by mmap writes.
> + *
> + * Must be called with inode_lock(inode) held and without ui->ui_mutex held:
> + * we block on the old-EOF folio lock, and ->writepage() and ->page_mkwrite()
> + * hold that folio lock while taking ui_mutex; the reverse order deadlocks.
> + */
> +static void ubifs_zero_eof_hole(struct inode *inode, loff_t new_end)
> +{
> +	loff_t old_size = i_size_read(inode);
> +	size_t from, to;
> +	struct folio *folio;
> +
> +	if (new_end <= old_size)
> +		return;
> +
> +	folio = filemap_lock_folio(inode->i_mapping, old_size >> PAGE_SHIFT);
> +	if (IS_ERR(folio))
> +		return;
> +
> +	from = offset_in_folio(folio, old_size);
> +	if (!from)
> +		goto out;
> +	to = folio_contains(folio, new_end >> PAGE_SHIFT) ?
> +	     offset_in_folio(folio, new_end) : folio_size(folio);
> +	if (to > from)
> +		folio_zero_range(folio, from, to - from);
> +
> +out:
> +	folio_unlock(folio);
> +	folio_put(folio);
> +}

[ ... ]

> @@ -1433,11 +1476,46 @@ static int update_mctime(struct inode *inode)
>  
>  static ssize_t ubifs_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  {
> -	int err = update_mctime(file_inode(iocb->ki_filp));
> -	if (err)
> -		return err;
> +	struct inode *inode = file_inode(iocb->ki_filp);
> +	ssize_t ret;
> +
> +	if (iocb->ki_flags & IOCB_NOWAIT)
> +		return -EOPNOTSUPP;
> +
> +	inode_lock(inode);
> +
> +	ret = generic_write_checks(iocb, from);
> +	if (ret <= 0)
> +		goto out;
> +
> +	ret = kiocb_modified(iocb);
> +	if (ret)
> +		goto out;
>  
> -	return generic_file_write_iter(iocb, from);
> +	/*
> +	 * UBIFS sets S_NOCMTIME on all inodes, so VFS's file_update_time_flags()
> +	 * short-circuits and never calls ubifs_update_time() for mtime/ctime.
> +	 * We must call update_mctime() explicitly here.
> +	 */
> +	ret = update_mctime(inode);
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * If the write position is beyond EOF, zero the stale tail of the
> +	 * old-EOF folio first.  generic_write_checks() has already resolved
> +	 * O_APPEND/RWF_APPEND and rejected invalid writes, so iocb->ki_pos
> +	 * is the final write position.
> +	 */
> +	if (iocb->ki_pos > i_size_read(inode))
> +		ubifs_zero_eof_hole(inode, iocb->ki_pos);
> +
> +	ret = generic_perform_write(iocb, from);

[Severity: High]
Can stale mmap data still be exposed when generic_perform_write() encounters a
short or zero-byte copy in copy_folio_from_iter_atomic()?

Suppose a file has a non-page-aligned size old_size and a shared mmap writes
beyond EOF, leaving the old-EOF folio uptodate with stale data in
[old_size, folio_size(folio)).

When a subsequent write() or pwrite() starts at pos >= old_size within that
same folio, ubifs_write_iter() skips ubifs_zero_eof_hole() if pos == old_size,
or only zeroes [old_size, pos) if pos > old_size, leaving [pos, folio_size(folio))
containing the stale mmap data under the assumption that the write will
overwrite [pos, pos + len).

However, if copy_folio_from_iter_atomic() copies fewer than len bytes (for
example, copied == 0 due to an unmapped user buffer returning -EFAULT, or an
unfaulted user page when a signal is pending), ubifs_write_end() only takes
the short-copy bailout when !folio_test_uptodate(folio), and otherwise extends
i_size to end_pos = pos + len rather than pos + copied:

fs/ubifs/file.c:ubifs_write_end() {
	loff_t end_pos = pos + len;
	int appending = !!(end_pos > inode->i_size);
	...
	if (unlikely(copied < len && !folio_test_uptodate(folio))) {
		...
		goto out;
	}
	...
	if (appending) {
		i_size_write(inode, end_pos);
		ui->ui_size = end_pos;
		...
	}
}

Because the old-EOF folio is already uptodate, wouldn't ubifs_write_end()
advance inode->i_size and ui->ui_size to pos + len even when copied < len,
exposing the unzeroed stale mmap data in [pos + copied, pos + len)?

By comparison, ext4_block_zero_eof() zeroes the entire tail of the old-EOF
folio from old_size to folio_size(folio), and ext4_write_end() only advances
i_size to pos + copied. Should ubifs_zero_eof_hole() also zero to the end of
the folio (or be called whenever the write extends past i_size), or should
ubifs_write_end() avoid extending i_size past pos + copied?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026125001.2570619-1-lin.nan@h3c.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/