Re: [PATCH] vboxsf: fix endless write loop and data corruption on short copy

[Hans de Goede]

+To: Jori Koolstra, who maintains vboxsf now.

Regards,

Hans

On 19-Sep-26 19:41, Kentaro Shiomi wrote:
> vboxsf_write_end() ignores the number of bytes that the generic write
> path managed to copy into the folio: it initialises nwritten with the
> requested length, writes that many bytes to the host and returns the
> requested length even when copied == 0.
> 
> generic_perform_write() then sees status != 0, so it never calls
> fault_in_iov_iter_readable(), advances pos by the full length and loops
> again with an iterator that has not been advanced at all.  The result is
> an endless loop that keeps appending zeroed data to the file (filling up
> the host file system) while flooding the log with
> 
>   WARNING: lib/iov_iter.c:624 at iov_iter_revert+0x1fc/0x270
> 
> because iov_iter_revert() is called with copied - status, i.e. a
> negative value.
> 
> The same accounting bug can silently corrupt data: when the folio is
> already uptodate, the stale part is not zeroed, so a short copy makes
> vboxsf write the old folio contents to the host and report success.
> 
> A short copy is not an error condition - it happens whenever the source
> pages are not faulted in yet, e.g. when writing directly from an mmap of
> another file or from shared memory (virtiofsd does exactly this).
> 
> Return the number of bytes that were actually copied, and reject the
> write entirely when nothing was copied so that the generic code faults
> the source pages in and retries.
> 
> Signed-off-by: Kentaro Shiomi <k.shiomi@xxxxxxxxxxxxxx>
> ---
> Found while running a nested VM (QEMU/KVM) inside a VirtualBox guest: the
> virtiofsd instance exporting a directory that lives on a vboxsf mount writes
> straight from the shared guest memory, so the source pages are not faulted in
> and every write takes the short-copy path.  Creating a 6-byte text file grew
> the file to 29 KB within seconds and a small PNG reached 202 MB before the VM
> was killed; the guest ran out of disk space because ~16 GB of logs were
> written in the meantime.
> 
> Reproduced without virtiofsd or nested virtualisation by writing a few bytes
> to a vboxsf file from a PROT_READ mapping (memfd or another file) that has not
> been read yet: with the mapping touched first the write succeeds, without it
> the write never returns and only dies on SIGKILL.
> 
> Tested on Ubuntu 26.04.1 (6.x userspace, kernel 7.0.0-31-generic) as a guest of
> VirtualBox 7.2.16 on a Windows host.  With the patch applied, writes from
> non-faulted pages, partially copied writes and in-place rewrites all produce
> byte-identical files on the host, and the virtiofsd workload completes with no
> kernel warnings.
> 
> While investigating I also hit an unrelated NULL pointer dereference in
> vboxsf_release_sf_handle() when opening a vboxsf file with O_DIRECT; that one
> will be reported separately.
> 
> A DKMS package with this patch is available at
> https://github.com/kentaro-shiomi/virtualbox-vboxsf-endless-write-loop-fix
>  fs/vboxsf/file.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/vboxsf/file.c b/fs/vboxsf/file.c
> index 7a7a3fbb2..1e9f61883 100644
> --- a/fs/vboxsf/file.c
> +++ b/fs/vboxsf/file.c
> @@ -307,7 +307,7 @@ static int vboxsf_write_end(const struct kiocb *iocb,
>  	struct inode *inode = mapping->host;
>  	struct vboxsf_handle *sf_handle = iocb->ki_filp->private_data;
>  	size_t from = offset_in_folio(folio, pos);
> -	u32 nwritten = len;
> +	u32 nwritten = copied;
>  	u8 *buf;
>  	int err;
>  
> @@ -315,6 +315,10 @@ static int vboxsf_write_end(const struct kiocb *iocb,
>  	if (!folio_test_uptodate(folio) && copied < len)
>  		folio_zero_range(folio, from + copied, len - copied);
>  
> +	/* Nothing copied: reject so generic_perform_write() faults in and retries */
> +	if (!copied)
> +		goto out;
> +
>  	buf = kmap(&folio->page);
>  	err = vboxsf_write(sf_handle->root, sf_handle->handle,
>  			   pos, &nwritten, buf + from);