| Message ID | 20240403215358.work.365-kees@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs: Set file_handle::handle_bytes before referencing file_handle::f_handle | expand |
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;
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 >
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
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 --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;
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(+)