[dm-devel] [bug report] dm crypt: conditionally enable code needed for tasklet usecases

[dm-devel] [bug report] dm crypt: conditionally enable code needed for tasklet usecases

[Dan Carpenter]

Hello Mike Snitzer,

The patch ae75a25bd83f: "dm crypt: conditionally enable code needed
for tasklet usecases" from Mar 6, 2023, leads to the following Smatch
static checker warning:

	drivers/md/dm-crypt.c:2758 crypt_dtr()
	warn: 'cc' was already freed.

drivers/md/dm-crypt.c
    2739         if (cc->dev)
    2740                 dm_put_device(ti, cc->dev);
    2741 
    2742         kfree_sensitive(cc->cipher_string);
    2743         kfree_sensitive(cc->key_string);
    2744         kfree_sensitive(cc->cipher_auth);
    2745         kfree_sensitive(cc->authenc_key);
    2746 
    2747         mutex_destroy(&cc->bio_alloc_lock);
    2748 
    2749         /* Must zero key material before freeing */
    2750         kfree_sensitive(cc);
                                 ^^

    2751 
    2752         spin_lock(&dm_crypt_clients_lock);
    2753         WARN_ON(!dm_crypt_clients_n);
    2754         dm_crypt_clients_n--;
    2755         crypt_calculate_pages_per_client();
    2756         spin_unlock(&dm_crypt_clients_lock);
    2757 
--> 2758         if (test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags) ||
                                                           ^^^^^^^^^
    2759             test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags))
                                                            ^^^^^^^^^
UAF.  This wasn't tested, right?  If this passes testing then it means
kfree_sensitive() is broken.  (Normally UAF bugs can only be detected
with KASan, but kfree_sensitive() should poison the data I thought).

    2760                 static_branch_dec(&use_tasklet_enabled);
    2761 
    2762         dm_audit_log_dtr(DM_MSG_PREFIX, ti, 1);
    2763 }

regards,
dan carpenter

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [bug report] dm crypt: conditionally enable code needed for tasklet usecases

[Dan Carpenter]

On Thu, Mar 09, 2023 at 05:35:20PM +0300, Dan Carpenter wrote:
> --> 2758         if (test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags) ||
>                                                            ^^^^^^^^^
>     2759             test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags))
>                                                             ^^^^^^^^^
> UAF.  This wasn't tested, right?  If this passes testing then it means
> kfree_sensitive() is broken.  (Normally UAF bugs can only be detected
> with KASan, but kfree_sensitive() should poison the data I thought).
> 

Nope.  This is thing where you need KASan to detect the bug.  I'm wrong
and continually demonstrate how even twenty years in to it I still don't
understand pointers.

regards,
dan carpenter

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [bug report] dm crypt: conditionally enable code needed for tasklet usecases

[Mike Snitzer]

On Thu, Mar 09 2023 at  9:42P -0500,
Dan Carpenter <error27@gmail.com> wrote:

> On Thu, Mar 09, 2023 at 05:35:20PM +0300, Dan Carpenter wrote:
> > --> 2758         if (test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags) ||
> >                                                            ^^^^^^^^^
> >     2759             test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags))
> >                                                             ^^^^^^^^^
> > UAF.  This wasn't tested, right?  If this passes testing then it means
> > kfree_sensitive() is broken.  (Normally UAF bugs can only be detected
> > with KASan, but kfree_sensitive() should poison the data I thought).
> > 
> 
> Nope.  This is thing where you need KASan to detect the bug.  I'm wrong
> and continually demonstrate how even twenty years in to it I still don't
> understand pointers.

Thanks for the report, really appreciate it. Sorry for the oversight
(and lack of testing). But we decided to fix a different way and
linux-next was updated accordingly, I just tweaked it but here is the
final:

https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=d9a02e016aaf5a57fb44e9a5e6da8ccd3b9e2e70

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel