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@xxxxxxxxxxx> 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@xxxxxxx>
>>>
>>> ---
>>> 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
