linux-kernel - Re: [PATCH] devtmpfs: move NULLing the thread pointer before unregistering fs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e676694f-6f6e-661d-d776-75d357ba510d@virtuozzo.com>
Date:   Fri, 2 Dec 2022 21:49:03 +0200
From:   Alexander Atanasov <alexander.atanasov@...tuozzo.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>, kernel@...nvz.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] devtmpfs: move NULLing the thread pointer before
 unregistering fs

On 2.12.22 17:56, Greg Kroah-Hartman wrote:
> On Fri, Dec 02, 2022 at 02:45:01PM +0200, Alexander Atanasov wrote:
>> In commit
>> 31c779f293b3 ("devtmpfs: fix the dangling pointer of global devtmpfsd thread")
>> a dangling pointer on an error condition was fixed. But the fix
>> left the dangling pointer during unregister_filesystem and printk calls.
> 
> And how could it be used there?

I don't said it can be used there - they might trigger events that get 
back to it.

> 
>> Improve the fix to clear the pointer before unregistration to close
>> the window  where the dangling pointer can be potentially used.
> 
> Again, how can that happen?  And you have an extra ' ' in that line :(

Sorry for the extra ' ' i will check where it came from.

> 
>> Make it clear the pointer at only one place in the function.
>>
>> Signed-off-by: Alexander Atanasov <alexander.atanasov@...tuozzo.com>
>> ---
>>   drivers/base/devtmpfs.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
>> index e4bffeabf344..773e66ef5642 100644
>> --- a/drivers/base/devtmpfs.c
>> +++ b/drivers/base/devtmpfs.c
>> @@ -472,17 +472,15 @@ int __init devtmpfs_init(void)
>>   	}
>>   
>>   	thread = kthread_run(devtmpfsd, &err, "kdevtmpfs");
>> -	if (!IS_ERR(thread)) {
>> +	if (!IS_ERR(thread))
>>   		wait_for_completion(&setup_done);
>> -	} else {
>> +	else
>>   		err = PTR_ERR(thread);
>> -		thread = NULL;
>> -	}
>>   
>>   	if (err) {
>> +		thread = NULL;
>>   		printk(KERN_ERR "devtmpfs: unable to create devtmpfs %i\n", err);
>>   		unregister_filesystem(&dev_fs_type);
>> -		thread = NULL;
>>   		return err;
>>   	}
> 
> This all feels wrong and way too complex to have to clean up from a call
> to kthread_run().  Are you sure this is the correct way to do this?

Agree on this but this is the code as it is.

> And how was this "issue" found?  How does the call to kthread_run() ever
> fail for you?

I was after something else and this stuck into my eye:
....
         thread = kthread_run(devtmpfsd, &err, "kdevtmpfs");
         if (!IS_ERR(thread)) {
                 wait_for_completion(&setup_done);
         } else {
                 err = PTR_ERR(thread);
                 thread = NULL;
         }

         if (err) {
                 printk(KERN_ERR "devtmpfs: unable to create devtmpfs 
%i\n", err);
                 unregister_filesystem(&dev_fs_type);
                 thread = NULL;
                 return err;
         }
....

Why do we do thread = NULL twice ? One time before unregistration, one 
time after unregistration.

So if it is going to handle the error the same way as the kthread_run 
error (original) then when the thread completes with error we must do 
the same. And do it one time.

...
         thread = kthread_run(devtmpfsd, &err, "kdevtmpfs");
         if (!IS_ERR(thread))
                 wait_for_completion(&setup_done);
         else
                 err = PTR_ERR(thread);

         if (err) {
                 thread = NULL;
                 printk(KERN_ERR "devtmpfs: unable to create devtmpfs 
%i\n", err);
                 unregister_filesystem(&dev_fs_type);
                 return err;
         }
...

Which is more readable ?

I guess  I should have put this as the commit message.

-- 
Regards,
Alexander Atanasov