Re: [PATCH] usb: gadget: f_fs: Fix setting of device and driver data cross-references

[Eugeniu Rosca <erosca@de.adit-jv.com>]

Hello,

On Thu, Jun 03, 2021 at 12:15:07PM -0500, Andrew Gabbasov wrote:
> FunctionFS device structure 'struct ffs_dev' and driver data structure
> 'struct ffs_data' are bound to each other with cross-reference pointers
> 'ffs_data->private_data' and 'ffs_dev->ffs_data'. While the first one
> is supposed to be valid through the whole life of 'struct ffs_data'
> (and while 'struct ffs_dev' exists non-freed), the second one is cleared
> in 'ffs_closed()' (called from 'ffs_data_reset()' or the last
> 'ffs_data_put()'). This can be called several times, alternating in
> different order with 'ffs_free_inst()', that, if possible, clears
> the other cross-reference.
> 
> As a result, different cases of these calls order may leave stale
> cross-reference pointers, used when the pointed structure is already
> freed. Even if it occasionally doesn't cause kernel crash, this error
> is reported by KASAN-enabled kernel configuration.
> 
> For example, the case [last 'ffs_data_put()' - 'ffs_free_inst()'] was
> fixed by commit cdafb6d8b8da ("usb: gadget: f_fs: Fix use-after-free in
> ffs_free_inst").
> 
> The other case ['ffs_data_reset()' - 'ffs_free_inst()' - 'ffs_data_put()']
> now causes KASAN reported error [1], when 'ffs_data_reset()' clears
> 'ffs_dev->ffs_data', then 'ffs_free_inst()' frees the 'struct ffs_dev',
> but can't clear 'ffs_data->private_data', which is then accessed
> in 'ffs_closed()' called from 'ffs_data_put()'. This happens since
> 'ffs_dev->ffs_data' reference is cleared too early.
> 
> Moreover, one more use case, when 'ffs_free_inst()' is called immediately
> after mounting FunctionFS device (that is before the descriptors are
> written and 'ffs_ready()' is called), and then 'ffs_data_reset()'
> or 'ffs_data_put()' is called from accessing "ep0" file or unmounting
> the device. This causes KASAN error report like [2], since
> 'ffs_dev->ffs_data' is not yet set when 'ffs_free_inst()' can't properly
> clear 'ffs_data->private_data', that is later accessed to freed structure.

I confirm there are at least two KASAN use-after-free issues
consistently/100% reproducible on v5.13-rc4-88-gf88cd3fb9df2:

https://gist.github.com/erosca/b5976a96789e574b319cb9e076938b5c
https://gist.github.com/erosca/4ded55ed32f0133bc2f4ccfe821c7776

These two can no longer be seen after the patch is applied.

In addition, below static analysis tools did not spot any regressions:
cppcheck 2.4, smatch v0.5.0-7445-g58776ae33ae8, make W=1, coccicheck

Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>

-- 
Best regards,
Eugeniu Rosca