diff mbox series

fs: Set file_handle::handle_bytes before referencing file_handle::f_handle

Message ID 20240403215358.work.365-kees@kernel.org (mailing list archive)
State Superseded
Headers show
Series fs: Set file_handle::handle_bytes before referencing file_handle::f_handle | expand

Commit Message

Kees Cook April 3, 2024, 9:54 p.m. UTC
With adding __counted_by(handle_bytes) to struct file_handle, we need
to explicitly set it in the one place it wasn't yet happening prior to
accessing the flex array "f_handle".

Fixes: 1b43c4629756 ("fs: Annotate struct file_handle with __counted_by() and use struct_size()")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: Christian Brauner <brauner@kernel.org>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Jan Kara <jack@suse.cz>
Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-nfs@vger.kernel.org
Cc: linux-hardening@vger.kernel.org
---
 fs/fhandle.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Gustavo A. R. Silva April 3, 2024, 10:11 p.m. UTC | #1
On 03/04/24 15:54, Kees Cook wrote:
> With adding __counted_by(handle_bytes) to struct file_handle, we need
> to explicitly set it in the one place it wasn't yet happening prior to
> accessing the flex array "f_handle".

Yes, which (access to `f_handle`) happens here:

  48         retval = exportfs_encode_fh(path->dentry,
  49                                     (struct fid *)handle->f_handle,
  50                                     &handle_dwords, fh_flags);

> 
> Fixes: 1b43c4629756 ("fs: Annotate struct file_handle with __counted_by() and use struct_size()")
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks for catching this!
--
Gustavo

> ---
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-nfs@vger.kernel.org
> Cc: linux-hardening@vger.kernel.org
> ---
>   fs/fhandle.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 53ed54711cd2..08ec2340dd22 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -40,6 +40,7 @@ static long do_sys_name_to_handle(const struct path *path,
>   			 GFP_KERNEL);
>   	if (!handle)
>   		return -ENOMEM;
> +	handle->handle_bytes = f_handle.handle_bytes;
>   
>   	/* convert handle size to multiple of sizeof(u32) */
>   	handle_dwords = f_handle.handle_bytes >> 2;
Jan Kara April 4, 2024, 9:19 a.m. UTC | #2
On Wed 03-04-24 14:54:03, Kees Cook wrote:
> With adding __counted_by(handle_bytes) to struct file_handle, we need
> to explicitly set it in the one place it wasn't yet happening prior to
> accessing the flex array "f_handle".
> 
> Fixes: 1b43c4629756 ("fs: Annotate struct file_handle with __counted_by() and use struct_size()")
> Signed-off-by: Kees Cook <keescook@chromium.org>

OK, so this isn't really a functional bug AFAIU but the compiler will
wrongly complain we are accessing handle->f_handle beyond claimed array
size (because handle->handle_bytes == 0 at that point). Am I right? If
that's the case, please add a short comment explaining this (because it
looks odd we set handle->handle_bytes and then reset it a few lines later).
With the comment feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-nfs@vger.kernel.org
> Cc: linux-hardening@vger.kernel.org
> ---
>  fs/fhandle.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 53ed54711cd2..08ec2340dd22 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -40,6 +40,7 @@ static long do_sys_name_to_handle(const struct path *path,
>  			 GFP_KERNEL);
>  	if (!handle)
>  		return -ENOMEM;
> +	handle->handle_bytes = f_handle.handle_bytes;
>  
>  	/* convert handle size to multiple of sizeof(u32) */
>  	handle_dwords = f_handle.handle_bytes >> 2;
> -- 
> 2.34.1
>
Chuck Lever III April 4, 2024, 3:25 p.m. UTC | #3
On Thu, Apr 04, 2024 at 11:19:00AM +0200, Jan Kara wrote:
> On Wed 03-04-24 14:54:03, Kees Cook wrote:
> > With adding __counted_by(handle_bytes) to struct file_handle, we need
> > to explicitly set it in the one place it wasn't yet happening prior to
> > accessing the flex array "f_handle".
> > 
> > Fixes: 1b43c4629756 ("fs: Annotate struct file_handle with __counted_by() and use struct_size()")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> OK, so this isn't really a functional bug AFAIU but the compiler will
> wrongly complain we are accessing handle->f_handle beyond claimed array
> size (because handle->handle_bytes == 0 at that point). Am I right? If
> that's the case, please add a short comment explaining this (because it
> looks odd we set handle->handle_bytes and then reset it a few lines later).
> With the comment feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								Honza

I agree, an in-code comment is needed.

Acked-by: Chuck Lever <chuck.lever@oracle.com>


> > ---
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Chuck Lever <chuck.lever@oracle.com>
> > Cc: Jeff Layton <jlayton@kernel.org>
> > Cc: Amir Goldstein <amir73il@gmail.com>
> > Cc: linux-fsdevel@vger.kernel.org
> > Cc: linux-nfs@vger.kernel.org
> > Cc: linux-hardening@vger.kernel.org
> > ---
> >  fs/fhandle.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > index 53ed54711cd2..08ec2340dd22 100644
> > --- a/fs/fhandle.c
> > +++ b/fs/fhandle.c
> > @@ -40,6 +40,7 @@ static long do_sys_name_to_handle(const struct path *path,
> >  			 GFP_KERNEL);
> >  	if (!handle)
> >  		return -ENOMEM;
> > +	handle->handle_bytes = f_handle.handle_bytes;
> >  
> >  	/* convert handle size to multiple of sizeof(u32) */
> >  	handle_dwords = f_handle.handle_bytes >> 2;
> > -- 
> > 2.34.1
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
Christian Brauner April 5, 2024, 11 a.m. UTC | #4
On Thu, Apr 04, 2024 at 11:19:00AM +0200, Jan Kara wrote:
> On Wed 03-04-24 14:54:03, Kees Cook wrote:
> > With adding __counted_by(handle_bytes) to struct file_handle, we need
> > to explicitly set it in the one place it wasn't yet happening prior to
> > accessing the flex array "f_handle".
> > 
> > Fixes: 1b43c4629756 ("fs: Annotate struct file_handle with __counted_by() and use struct_size()")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> OK, so this isn't really a functional bug AFAIU but the compiler will
> wrongly complain we are accessing handle->f_handle beyond claimed array
> size (because handle->handle_bytes == 0 at that point). Am I right? If

And really, this also needs to please be mentioned in the commit message
because from reading the commit message I'm not even sure what this
patch is trying to fix.
diff mbox series

Patch

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 53ed54711cd2..08ec2340dd22 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -40,6 +40,7 @@  static long do_sys_name_to_handle(const struct path *path,
 			 GFP_KERNEL);
 	if (!handle)
 		return -ENOMEM;
+	handle->handle_bytes = f_handle.handle_bytes;
 
 	/* convert handle size to multiple of sizeof(u32) */
 	handle_dwords = f_handle.handle_bytes >> 2;