| Message ID | 20240408075052.3304511-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] orangefs: fix out-of-bounds fsid access | expand |
On Mon 08-04-24 09:50:43, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > orangefs_statfs() copies two consecutive fields of the superblock into > the statfs structure, which triggers a warning from the string fortification > helpers: > > In file included from fs/orangefs/super.c:8: > include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning] > __read_overflow2_field(q_size_field, size); > > Change the memcpy() to an individual assignment of the two fields, which helps > both the compiler and human readers understand better what it does. > > Link: https://lore.kernel.org/all/20230622101701.3399585-1-arnd@kernel.org/ > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Jan Kara <jack@suse.cz> > Cc: linux-fsdevel@vger.kernel.org > Cc: Mike Marshall <hubcap@omnibond.com> > Cc: Martin Brandenburg <martin@omnibond.com> > Cc: devel@lists.orangefs.org > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Resending to VFS maintainers, I sent this a couple of times to the > orangefs maintainers but never got a reply > --- > fs/orangefs/super.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c > index fb4d09c2f531..152478295766 100644 > --- a/fs/orangefs/super.c > +++ b/fs/orangefs/super.c > @@ -201,7 +201,10 @@ static int orangefs_statfs(struct dentry *dentry, struct kstatfs *buf) > (long)new_op->downcall.resp.statfs.files_avail); > > buf->f_type = sb->s_magic; > - memcpy(&buf->f_fsid, &ORANGEFS_SB(sb)->fs_id, sizeof(buf->f_fsid)); > + buf->f_fsid = (__kernel_fsid_t) {{ > + ORANGEFS_SB(sb)->fs_id, > + ORANGEFS_SB(sb)->id, > + }}; Frankly, this initializer is hard to understand for me. Why not simple: buf->f_fsid[0] = ORANGEFS_SB(sb)->fs_id; buf->f_fsid[1] = ORANGEFS_SB(sb)->id; Honza
On Mon, Apr 8, 2024 at 7:36 AM Jan Kara <jack@suse.cz> wrote: > Frankly, this initializer is hard to understand for me. Why not simple: > > buf->f_fsid[0] = ORANGEFS_SB(sb)->fs_id; > buf->f_fsid[1] = ORANGEFS_SB(sb)->id; > +1 for this idea, seems easier to read for me. > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
On Mon, Apr 8, 2024, at 23:21, Justin Stitt wrote: > On Mon, Apr 8, 2024 at 7:36 AM Jan Kara <jack@suse.cz> wrote: >> Frankly, this initializer is hard to understand for me. Why not simple: >> >> buf->f_fsid[0] = ORANGEFS_SB(sb)->fs_id; >> buf->f_fsid[1] = ORANGEFS_SB(sb)->id; >> > > +1 for this idea, seems easier to read for me. Yes, good idea, I'll send this as v2 after my next round of build testing. Arnd
I applied Arnd's patch on top of Linux 6.9-rc3 and ran through xfstests with no issue. Also, instead of Arnd's patch, I used Jan's idea: + + buf->f_fsid.val[0] = ORANGEFS_SB(sb)->fs_id; + buf->f_fsid.val[1] = ORANGEFS_SB(sb)->id; + And ran that through as well, no issue. Sorry for missing the earlier patch. -Mike On Tue, Apr 9, 2024 at 1:55 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Mon, Apr 8, 2024, at 23:21, Justin Stitt wrote: > > On Mon, Apr 8, 2024 at 7:36 AM Jan Kara <jack@suse.cz> wrote: > >> Frankly, this initializer is hard to understand for me. Why not simple: > >> > >> buf->f_fsid[0] = ORANGEFS_SB(sb)->fs_id; > >> buf->f_fsid[1] = ORANGEFS_SB(sb)->id; > >> > > > > +1 for this idea, seems easier to read for me. > > Yes, good idea, I'll send this as v2 after my next round > of build testing. > > Arnd
On Tue, Apr 9, 2024, at 18:26, Mike Marshall wrote: > I applied Arnd's patch on top of Linux 6.9-rc3 and > ran through xfstests with no issue. > > Also, instead of Arnd's patch, I used Jan's idea: > > + > + buf->f_fsid.val[0] = ORANGEFS_SB(sb)->fs_id; > + buf->f_fsid.val[1] = ORANGEFS_SB(sb)->id; > + > > And ran that through as well, no issue. > > Sorry for missing the earlier patch. Thanks! I was about to send the updated patch and can skip that now. Arnd
diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c index fb4d09c2f531..152478295766 100644 --- a/fs/orangefs/super.c +++ b/fs/orangefs/super.c @@ -201,7 +201,10 @@ static int orangefs_statfs(struct dentry *dentry, struct kstatfs *buf) (long)new_op->downcall.resp.statfs.files_avail); buf->f_type = sb->s_magic; - memcpy(&buf->f_fsid, &ORANGEFS_SB(sb)->fs_id, sizeof(buf->f_fsid)); + buf->f_fsid = (__kernel_fsid_t) {{ + ORANGEFS_SB(sb)->fs_id, + ORANGEFS_SB(sb)->id, + }}; buf->f_bsize = new_op->downcall.resp.statfs.block_size; buf->f_namelen = ORANGEFS_NAME_MAX;