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

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

[Kentaro Shiomi]

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@techhowto.blog>
---
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);
-- 
2.53.0

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@techhowto.blog>
> ---
> 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);