gpioc: Fix handling of priv data during open

[thj]

Summary

If the gpioc open call fails devfs_clear_cdevpriv will be called
automatically for us, calling the destructor. If we call this in an
error path this can lead to a use after free when repeatidly opening
gpioc and listening for interrupts.

devfs_set_cdevpriv can fail, return the value of this call so when it
does fail our destructor is called for us.

Test Plan

On imx8mp with this change we are able to survive this stress test (using
tools/test/gpioevents):

for x in $(seq 1 100) ; do echo $x; timeout 1 ./gpioevents 1 er pd; done

[markj]

I don't really understand this change.

devfs_set_cdevpriv can fail, return the value of this call so when it does fail our destructor is called for us.

But devfs_set_cdevpriv() is what sets up the destructor. It fails when a cdevpriv is already registered for this file handle. (How exactly is that happening here?)

I think the old code has a bug in that it may invoke the destructor before priv is fully initialized (e.g., the destructor will call mtx_destroy(&priv->mtx) with priv->mtx uninitialized), but if devfs_set_cdevpriv() fails, it's still up to gpioc_open() to destroy priv.

[jhb]

I think you either want to just replace gpioc_cdevpriv_dtor(priv) in the original code with just free(priv, M_GPIOC), or you do indeed want to defer the call to devfs_set_cdevpriv until after priv is fully constructed. I think in general I would prefer the latter approach of fully constructing an object before handing it off. However, as Mark notes, you do have to explicitly cleanup if devfs_set_cdevpriv fails as otherwise you are leaking. (Also, style(9) wants ()'s around the argument to return.)

However, I also see other things here that are concerning to me in that priv doesn't seem fully initialized when it is created. I guess SLIST_INIT() just sets sl_first to NULL so that is implied by M_ZERO but I'd really rather that be explicitly done. If pins was ever converted to a different queue macro that would be a pretty quick bug to trip over.

[thj]

• handle devfs_set_cdevpriv correctly
• actively initialize more of priv

[thj]

Thanks for the comments both, I'm sorry I rolled my confusion about the use of cdevpriv and clean up with this change.

Through the tree and the man page I found 3 different uses of automatic free'ing of cdevpriv on failure:

• it is automatic
• don't check the result devfs_set_cdevpriv
• check result and call destructor or manually clean up

From your comments it is clear that users should be tidying up, even if the potential leak is rare/impossible.