| Message ID | b923f900-3e09-4c6e-a199-05053376d7c2@fastmail.fm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Avoid fuse_file_args null pointer dereference | expand |
On Mon, 22 Apr 2024 at 19:40, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > The test for NULL was done for the member of union fuse_file_args, > but not for fuse_file_args itself. > > Fixes: e26ee4efbc796 ("fuse: allocate ff->release_args only if release is needed") > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > > --- > I'm currently going through all the recent patches again and noticed > in code review. I guess this falls through testing, because we don't > run xfstests that have !fc->no_opendir || !fc->no_open. > > Note: Untested except that it compiles. > > Note2: Our IT just broke sendmail, I'm quickly sending through thunderbird, > I hope doesn't change the patch format. > > fs/fuse/file.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index b57ce4157640..0ff865457ea6 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -102,7 +102,8 @@ static void fuse_release_end(struct fuse_mount *fm, struct fuse_args *args, > static void fuse_file_put(struct fuse_file *ff, bool sync) > { > if (refcount_dec_and_test(&ff->count)) { > - struct fuse_release_args *ra = &ff->args->release_args; > + struct fuse_release_args *ra = > + ff->args ? &ff->args->release_args : NULL; While this looks like a NULL pointer dereference, it isn't, because &foo->bar is just pointer arithmetic, and in this case the pointers will be identical. So it will work, but the whole ff->args thing is a bit confusing. Not sure how to properly clean this up, your patch seems to be just adding more obfuscation. Thanks, Miklos
On 4/23/24 12:46, Miklos Szeredi wrote: > On Mon, 22 Apr 2024 at 19:40, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: >> >> The test for NULL was done for the member of union fuse_file_args, >> but not for fuse_file_args itself. >> >> Fixes: e26ee4efbc796 ("fuse: allocate ff->release_args only if release is needed") >> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >> >> --- >> I'm currently going through all the recent patches again and noticed >> in code review. I guess this falls through testing, because we don't >> run xfstests that have !fc->no_opendir || !fc->no_open. >> >> Note: Untested except that it compiles. >> >> Note2: Our IT just broke sendmail, I'm quickly sending through thunderbird, >> I hope doesn't change the patch format. >> >> fs/fuse/file.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c >> index b57ce4157640..0ff865457ea6 100644 >> --- a/fs/fuse/file.c >> +++ b/fs/fuse/file.c >> @@ -102,7 +102,8 @@ static void fuse_release_end(struct fuse_mount *fm, struct fuse_args *args, >> static void fuse_file_put(struct fuse_file *ff, bool sync) >> { >> if (refcount_dec_and_test(&ff->count)) { >> - struct fuse_release_args *ra = &ff->args->release_args; >> + struct fuse_release_args *ra = >> + ff->args ? &ff->args->release_args : NULL; > > While this looks like a NULL pointer dereference, it isn't, because > &foo->bar is just pointer arithmetic, and in this case the pointers > will be identical. So it will work, but the whole ff->args thing is a > bit confusing. Not sure how to properly clean this up, your patch > seems to be just adding more obfuscation. Hmm, right, I had actually thought about that and written a small test, before creating the patch. But then had it slightly different - caused the null-ptr deref. Updated code works, but UBSAN still complains. bschubert2@imesrv6 test>./test-union test-union.c:23:10: runtime error: member access within null pointer of type 'union bar' No ptr cat test-union.c #include <stdio.h> #include <string.h> #include <stdlib.h> #include <assert.h> union bar { int foo; int foo2; }; struct test { union bar *bar; }; int main(void) { struct test *test = calloc(1, sizeof(test)); assert(test); int *foo_ptr = &test->bar->foo; if (foo_ptr) printf("Have ptr\n"); else printf("No ptr\n"); free(test); return 0; } Thanks, Bernd
On 4/23/24 14:23, Bernd Schubert wrote: > > > On 4/23/24 12:46, Miklos Szeredi wrote: >> On Mon, 22 Apr 2024 at 19:40, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: >>> >>> The test for NULL was done for the member of union fuse_file_args, >>> but not for fuse_file_args itself. >>> >>> Fixes: e26ee4efbc796 ("fuse: allocate ff->release_args only if release is needed") >>> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >>> >>> --- >>> I'm currently going through all the recent patches again and noticed >>> in code review. I guess this falls through testing, because we don't >>> run xfstests that have !fc->no_opendir || !fc->no_open. >>> >>> Note: Untested except that it compiles. >>> >>> Note2: Our IT just broke sendmail, I'm quickly sending through thunderbird, >>> I hope doesn't change the patch format. >>> >>> fs/fuse/file.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c >>> index b57ce4157640..0ff865457ea6 100644 >>> --- a/fs/fuse/file.c >>> +++ b/fs/fuse/file.c >>> @@ -102,7 +102,8 @@ static void fuse_release_end(struct fuse_mount *fm, struct fuse_args *args, >>> static void fuse_file_put(struct fuse_file *ff, bool sync) >>> { >>> if (refcount_dec_and_test(&ff->count)) { >>> - struct fuse_release_args *ra = &ff->args->release_args; >>> + struct fuse_release_args *ra = >>> + ff->args ? &ff->args->release_args : NULL; >> >> While this looks like a NULL pointer dereference, it isn't, because >> &foo->bar is just pointer arithmetic, and in this case the pointers >> will be identical. So it will work, but the whole ff->args thing is a >> bit confusing. Not sure how to properly clean this up, your patch >> seems to be just adding more obfuscation. > > Hmm, right, I had actually thought about that and written a small test, > before creating the patch. But then had it slightly different - caused > the null-ptr deref. > Updated code works, but UBSAN still complains. > > > bschubert2@imesrv6 test>./test-union > test-union.c:23:10: runtime error: member access within null pointer of > type 'union bar' > No ptr > > > cat test-union.c > > #include <stdio.h> > #include <string.h> > #include <stdlib.h> > #include <assert.h> > > union bar > { > int foo; > int foo2; > }; > > struct test > { > union bar *bar; > }; > > int main(void) > { > struct test *test = calloc(1, sizeof(test)); > assert(test); > > int *foo_ptr = &test->bar->foo; > > if (foo_ptr) > printf("Have ptr\n"); > else > printf("No ptr\n"); > > free(test); > > return 0; > } Tested with an UBSAN enabled kernel and libfuse-hacked-in-no-opendir. No UBSAN warning - so sorry for the noise! At least I learned that libfuse by default doesn't deactivate open/opendir, I had never looked into that before. Thanks, Bernd
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index b57ce4157640..0ff865457ea6 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -102,7 +102,8 @@ static void fuse_release_end(struct fuse_mount *fm, struct fuse_args *args, static void fuse_file_put(struct fuse_file *ff, bool sync) { if (refcount_dec_and_test(&ff->count)) { - struct fuse_release_args *ra = &ff->args->release_args; + struct fuse_release_args *ra = + ff->args ? &ff->args->release_args : NULL; struct fuse_args *args = (ra ? &ra->args : NULL); if (ra && ra->inode) @@ -292,7 +293,7 @@ static void fuse_prepare_release(struct fuse_inode *fi, struct fuse_file *ff, unsigned int flags, int opcode, bool sync) { struct fuse_conn *fc = ff->fm->fc; - struct fuse_release_args *ra = &ff->args->release_args; + struct fuse_release_args *ra = ff->args ? &ff->args->release_args : NULL; if (fuse_file_passthrough(ff)) fuse_passthrough_release(ff, fuse_inode_backing(fi)); @@ -337,7 +338,7 @@ void fuse_file_release(struct inode *inode, struct fuse_file *ff, unsigned int open_flags, fl_owner_t id, bool isdir) { struct fuse_inode *fi = get_fuse_inode(inode); - struct fuse_release_args *ra = &ff->args->release_args; + struct fuse_release_args *ra = ff->args ? &ff->args->release_args : NULL; int opcode = isdir ? FUSE_RELEASEDIR : FUSE_RELEASE; fuse_prepare_release(fi, ff, open_flags, opcode, false);
The test for NULL was done for the member of union fuse_file_args, but not for fuse_file_args itself. Fixes: e26ee4efbc796 ("fuse: allocate ff->release_args only if release is needed") Signed-off-by: Bernd Schubert <bschubert@ddn.com> --- I'm currently going through all the recent patches again and noticed in code review. I guess this falls through testing, because we don't run xfstests that have !fc->no_opendir || !fc->no_open. Note: Untested except that it compiles. Note2: Our IT just broke sendmail, I'm quickly sending through thunderbird, I hope doesn't change the patch format. fs/fuse/file.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)