Add validation in virtio-fs to ensure the server follows the FUSE
protocol for response headers, addressing the existing TODO for
verifying protocol compliance.

Add checks for fuse_out_header to verify:
 - oh->unique matches req->in.h.unique
 - FUSE_INT_REQ_BIT is not set
 - error codes are valid
 - oh->len does not exceed the expected size

Signed-off-by: Brajesh Patil <brajeshpatil11@gmail.com>
---
 fs/fuse/virtio_fs.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 6bc7c97b017d..52e8338bf436 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -764,14 +764,34 @@ static void virtio_fs_request_complete(struct fuse_req *req,
 {
 	struct fuse_args *args;
 	struct fuse_args_pages *ap;
-	unsigned int len, i, thislen;
+	struct fuse_out_header *oh;
+	unsigned int len, i, thislen, expected_len = 0;
 	struct folio *folio;
 
-	/*
-	 * TODO verify that server properly follows FUSE protocol
-	 * (oh.uniq, oh.len)
-	 */
+	oh = &req->out.h;
+
+	if (oh->unique == 0)
+		pr_warn_once("notify through fuse-virtio-fs not supported");
+
+	if ((oh->unique & ~FUSE_INT_REQ_BIT) != req->in.h.unique)
+		pr_warn_ratelimited("virtio-fs: unique mismatch, expected: %llu got %llu\n",
+				    req->in.h.unique, oh->unique & ~FUSE_INT_REQ_BIT);
+
+	WARN_ON_ONCE(oh->unique & FUSE_INT_REQ_BIT);
+
+	if (oh->error <= -ERESTARTSYS || oh->error > 0)
+		pr_warn_ratelimited("virtio-fs: invalid error code from server: %d\n",
+				    oh->error);
+
 	args = req->args;
+
+	for (i = 0; i < args->out_numargs; i++)
+		expected_len += args->out_args[i].size;
+
+	if (oh->len > sizeof(*oh) + expected_len)
+		pr_warn("FUSE reply too long! got=%u expected<=%u\n",
+			oh->len, (unsigned int)(sizeof(*oh) + expected_len));
+
 	copy_args_from_argbuf(args, req);
 
 	if (args->out_pages && args->page_zeroing) {
-- 
2.43.0

On Wed, Oct 29, 2025 at 01:33:11AM +0530, Brajesh Patil wrote:
> Add validation in virtio-fs to ensure the server follows the FUSE
> protocol for response headers, addressing the existing TODO for
> verifying protocol compliance.
> 
> Add checks for fuse_out_header to verify:
>  - oh->unique matches req->in.h.unique
>  - FUSE_INT_REQ_BIT is not set
>  - error codes are valid
>  - oh->len does not exceed the expected size
> 
> Signed-off-by: Brajesh Patil <brajeshpatil11@gmail.com>
> ---
>  fs/fuse/virtio_fs.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 6bc7c97b017d..52e8338bf436 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -764,14 +764,34 @@ static void virtio_fs_request_complete(struct fuse_req *req,
>  {
>  	struct fuse_args *args;
>  	struct fuse_args_pages *ap;
> -	unsigned int len, i, thislen;
> +	struct fuse_out_header *oh;
> +	unsigned int len, i, thislen, expected_len = 0;
>  	struct folio *folio;
>  
> -	/*
> -	 * TODO verify that server properly follows FUSE protocol
> -	 * (oh.uniq, oh.len)
> -	 */
> +	oh = &req->out.h;
> +
> +	if (oh->unique == 0)
> +		pr_warn_once("notify through fuse-virtio-fs not supported");
> +
> +	if ((oh->unique & ~FUSE_INT_REQ_BIT) != req->in.h.unique)
> +		pr_warn_ratelimited("virtio-fs: unique mismatch, expected: %llu got %llu\n",
> +				    req->in.h.unique, oh->unique & ~FUSE_INT_REQ_BIT);

Er... shouldn't these be rejecting the response somehow?  Instead of
warning that something's amiss but continuing with known bad data?

--D

> +
> +	WARN_ON_ONCE(oh->unique & FUSE_INT_REQ_BIT);
> +
> +	if (oh->error <= -ERESTARTSYS || oh->error > 0)
> +		pr_warn_ratelimited("virtio-fs: invalid error code from server: %d\n",
> +				    oh->error);
> +
>  	args = req->args;
> +
> +	for (i = 0; i < args->out_numargs; i++)
> +		expected_len += args->out_args[i].size;
> +
> +	if (oh->len > sizeof(*oh) + expected_len)
> +		pr_warn("FUSE reply too long! got=%u expected<=%u\n",
> +			oh->len, (unsigned int)(sizeof(*oh) + expected_len));
> +
>  	copy_args_from_argbuf(args, req);
>  
>  	if (args->out_pages && args->page_zeroing) {
> -- 
> 2.43.0
> 
>

On Tue, Oct 28, 2025 at 01:07:55PM -0700, Darrick J. Wong wrote:
> On Wed, Oct 29, 2025 at 01:33:11AM +0530, Brajesh Patil wrote:
> > Add validation in virtio-fs to ensure the server follows the FUSE
> > protocol for response headers, addressing the existing TODO for
> > verifying protocol compliance.
> > 
> > Add checks for fuse_out_header to verify:
> >  - oh->unique matches req->in.h.unique
> >  - FUSE_INT_REQ_BIT is not set
> >  - error codes are valid
> >  - oh->len does not exceed the expected size
> > 
> > Signed-off-by: Brajesh Patil <brajeshpatil11@gmail.com>
> > ---
> >  fs/fuse/virtio_fs.c | 30 +++++++++++++++++++++++++-----
> >  1 file changed, 25 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > index 6bc7c97b017d..52e8338bf436 100644
> > --- a/fs/fuse/virtio_fs.c
> > +++ b/fs/fuse/virtio_fs.c
> > @@ -764,14 +764,34 @@ static void virtio_fs_request_complete(struct fuse_req *req,
> >  {
> >  	struct fuse_args *args;
> >  	struct fuse_args_pages *ap;
> > -	unsigned int len, i, thislen;
> > +	struct fuse_out_header *oh;
> > +	unsigned int len, i, thislen, expected_len = 0;
> >  	struct folio *folio;
> >  
> > -	/*
> > -	 * TODO verify that server properly follows FUSE protocol
> > -	 * (oh.uniq, oh.len)
> > -	 */
> > +	oh = &req->out.h;
> > +
> > +	if (oh->unique == 0)
> > +		pr_warn_once("notify through fuse-virtio-fs not supported");
> > +
> > +	if ((oh->unique & ~FUSE_INT_REQ_BIT) != req->in.h.unique)
> > +		pr_warn_ratelimited("virtio-fs: unique mismatch, expected: %llu got %llu\n",
> > +				    req->in.h.unique, oh->unique & ~FUSE_INT_REQ_BIT);
> 
> Er... shouldn't these be rejecting the response somehow?  Instead of
> warning that something's amiss but continuing with known bad data?
> 
> --D
>

Right, continuing here is unsafe.

I plan to update the code so that in case of any header validation
failure (e.g. unique mismatch, invalid error, length mismatch), it
should skip copying data and jump directly to the section that marks
request as complete

Does this seem like a feasible approach?

> > +
> > +	WARN_ON_ONCE(oh->unique & FUSE_INT_REQ_BIT);
> > +
> > +	if (oh->error <= -ERESTARTSYS || oh->error > 0)
> > +		pr_warn_ratelimited("virtio-fs: invalid error code from server: %d\n",
> > +				    oh->error);
> > +
> >  	args = req->args;
> > +
> > +	for (i = 0; i < args->out_numargs; i++)
> > +		expected_len += args->out_args[i].size;
> > +
> > +	if (oh->len > sizeof(*oh) + expected_len)
> > +		pr_warn("FUSE reply too long! got=%u expected<=%u\n",
> > +			oh->len, (unsigned int)(sizeof(*oh) + expected_len));
> > +
> >  	copy_args_from_argbuf(args, req);
> >  
> >  	if (args->out_pages && args->page_zeroing) {
> > -- 
> > 2.43.0
> > 
> >

On Wed, Oct 29, 2025 at 08:58:30AM +0530, Brajesh Patil wrote:
> On Tue, Oct 28, 2025 at 01:07:55PM -0700, Darrick J. Wong wrote:
> > On Wed, Oct 29, 2025 at 01:33:11AM +0530, Brajesh Patil wrote:
> > > Add validation in virtio-fs to ensure the server follows the FUSE
> > > protocol for response headers, addressing the existing TODO for
> > > verifying protocol compliance.
> > > 
> > > Add checks for fuse_out_header to verify:
> > >  - oh->unique matches req->in.h.unique
> > >  - FUSE_INT_REQ_BIT is not set
> > >  - error codes are valid
> > >  - oh->len does not exceed the expected size
> > > 
> > > Signed-off-by: Brajesh Patil <brajeshpatil11@gmail.com>
> > > ---
> > >  fs/fuse/virtio_fs.c | 30 +++++++++++++++++++++++++-----
> > >  1 file changed, 25 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > > index 6bc7c97b017d..52e8338bf436 100644
> > > --- a/fs/fuse/virtio_fs.c
> > > +++ b/fs/fuse/virtio_fs.c
> > > @@ -764,14 +764,34 @@ static void virtio_fs_request_complete(struct fuse_req *req,
> > >  {
> > >  	struct fuse_args *args;
> > >  	struct fuse_args_pages *ap;
> > > -	unsigned int len, i, thislen;
> > > +	struct fuse_out_header *oh;
> > > +	unsigned int len, i, thislen, expected_len = 0;
> > >  	struct folio *folio;
> > >  
> > > -	/*
> > > -	 * TODO verify that server properly follows FUSE protocol
> > > -	 * (oh.uniq, oh.len)
> > > -	 */
> > > +	oh = &req->out.h;
> > > +
> > > +	if (oh->unique == 0)
> > > +		pr_warn_once("notify through fuse-virtio-fs not supported");
> > > +
> > > +	if ((oh->unique & ~FUSE_INT_REQ_BIT) != req->in.h.unique)
> > > +		pr_warn_ratelimited("virtio-fs: unique mismatch, expected: %llu got %llu\n",
> > > +				    req->in.h.unique, oh->unique & ~FUSE_INT_REQ_BIT);
> > 
> > Er... shouldn't these be rejecting the response somehow?  Instead of
> > warning that something's amiss but continuing with known bad data?
> > 
> > --D
> >
> 
> Right, continuing here is unsafe.
> 
> I plan to update the code so that in case of any header validation
> failure (e.g. unique mismatch, invalid error, length mismatch), it
> should skip copying data and jump directly to the section that marks
> request as complete
> 
> Does this seem like a feasible approach?

Yeah, I think you can just set req->out.h.error to some errno (EIO?) and
jump to fuse_request_end, sort of like what fuse_dev_do_write sort of
does.  I think that sends the errno back to whatever code initiated the
request.  I don't know if virtiofs should be throwing an error back to
the server?

--D

> > > +
> > > +	WARN_ON_ONCE(oh->unique & FUSE_INT_REQ_BIT);
> > > +
> > > +	if (oh->error <= -ERESTARTSYS || oh->error > 0)
> > > +		pr_warn_ratelimited("virtio-fs: invalid error code from server: %d\n",
> > > +				    oh->error);
> > > +
> > >  	args = req->args;
> > > +
> > > +	for (i = 0; i < args->out_numargs; i++)
> > > +		expected_len += args->out_args[i].size;
> > > +
> > > +	if (oh->len > sizeof(*oh) + expected_len)
> > > +		pr_warn("FUSE reply too long! got=%u expected<=%u\n",
> > > +			oh->len, (unsigned int)(sizeof(*oh) + expected_len));
> > > +
> > >  	copy_args_from_argbuf(args, req);
> > >  
> > >  	if (args->out_pages && args->page_zeroing) {
> > > -- 
> > > 2.43.0
> > > 
> > >

On Tue, Oct 28, 2025 at 11:01:08PM -0700, Darrick J. Wong wrote:
> On Wed, Oct 29, 2025 at 08:58:30AM +0530, Brajesh Patil wrote:
> > On Tue, Oct 28, 2025 at 01:07:55PM -0700, Darrick J. Wong wrote:
> > > On Wed, Oct 29, 2025 at 01:33:11AM +0530, Brajesh Patil wrote:
> > > > Add validation in virtio-fs to ensure the server follows the FUSE
> > > > protocol for response headers, addressing the existing TODO for
> > > > verifying protocol compliance.
> > > > 
> > > > Add checks for fuse_out_header to verify:
> > > >  - oh->unique matches req->in.h.unique
> > > >  - FUSE_INT_REQ_BIT is not set
> > > >  - error codes are valid
> > > >  - oh->len does not exceed the expected size
> > > > 
> > > > Signed-off-by: Brajesh Patil <brajeshpatil11@gmail.com>
> > > > ---
> > > >  fs/fuse/virtio_fs.c | 30 +++++++++++++++++++++++++-----
> > > >  1 file changed, 25 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > > > index 6bc7c97b017d..52e8338bf436 100644
> > > > --- a/fs/fuse/virtio_fs.c
> > > > +++ b/fs/fuse/virtio_fs.c
> > > > @@ -764,14 +764,34 @@ static void virtio_fs_request_complete(struct fuse_req *req,
> > > >  {
> > > >  	struct fuse_args *args;
> > > >  	struct fuse_args_pages *ap;
> > > > -	unsigned int len, i, thislen;
> > > > +	struct fuse_out_header *oh;
> > > > +	unsigned int len, i, thislen, expected_len = 0;
> > > >  	struct folio *folio;
> > > >  
> > > > -	/*
> > > > -	 * TODO verify that server properly follows FUSE protocol
> > > > -	 * (oh.uniq, oh.len)
> > > > -	 */
> > > > +	oh = &req->out.h;
> > > > +
> > > > +	if (oh->unique == 0)
> > > > +		pr_warn_once("notify through fuse-virtio-fs not supported");
> > > > +
> > > > +	if ((oh->unique & ~FUSE_INT_REQ_BIT) != req->in.h.unique)
> > > > +		pr_warn_ratelimited("virtio-fs: unique mismatch, expected: %llu got %llu\n",
> > > > +				    req->in.h.unique, oh->unique & ~FUSE_INT_REQ_BIT);
> > > 
> > > Er... shouldn't these be rejecting the response somehow?  Instead of
> > > warning that something's amiss but continuing with known bad data?
> > > 
> > > --D
> > >
> > 
> > Right, continuing here is unsafe.
> > 
> > I plan to update the code so that in case of any header validation
> > failure (e.g. unique mismatch, invalid error, length mismatch), it
> > should skip copying data and jump directly to the section that marks
> > request as complete
> > 
> > Does this seem like a feasible approach?
> 
> Yeah, I think you can just set req->out.h.error to some errno (EIO?) and
> jump to fuse_request_end, sort of like what fuse_dev_do_write sort of
> does.  I think that sends the errno back to whatever code initiated the
> request.  I don't know if virtiofs should be throwing an error back to
> the server?
> 
> --D
> 

I think it is okay to set oh.error in fuse_dev_do_write as it is a server side
reply. But as virtio_fs is on the client side and oh.error has been set by
virtiofsd, I think so we should not overwrite oh.error. Instead, if we
encounter an error in any of the if conditions, skip copying arguments and jump
to the line clear_bit(FR_SENT, &req->flags).

> > > > +
> > > > +	WARN_ON_ONCE(oh->unique & FUSE_INT_REQ_BIT);
> > > > +
> > > > +	if (oh->error <= -ERESTARTSYS || oh->error > 0)
> > > > +		pr_warn_ratelimited("virtio-fs: invalid error code from server: %d\n",
> > > > +				    oh->error);
> > > > +
> > > >  	args = req->args;
> > > > +
> > > > +	for (i = 0; i < args->out_numargs; i++)
> > > > +		expected_len += args->out_args[i].size;
> > > > +
> > > > +	if (oh->len > sizeof(*oh) + expected_len)
> > > > +		pr_warn("FUSE reply too long! got=%u expected<=%u\n",
> > > > +			oh->len, (unsigned int)(sizeof(*oh) + expected_len));
> > > > +
> > > >  	copy_args_from_argbuf(args, req);
> > > >  
> > > >  	if (args->out_pages && args->page_zeroing) {
> > > > -- 
> > > > 2.43.0
> > > > 
> > > >