Fix use after free in msdosfs_rename()
AbandonedPublic
Actions

Authored by trasz on Nov 23 2020, 1:51 PM.

Details

Reviewers

Summary

Fix use after free in msdosfs_rename().

The ip points to v_data for fvp; however, fvp could have changed
during relookup(). This cannot happen if 'doingdirectory' is true,
and the DE_RENAME flag can only ever be set if 'doingdirectory'
is true, so use that to guard the clearing.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 34979
Build 31972: arc lint + arc unit

Event Timeline

trasz created this revision.Nov 23 2020, 1:51 PM

Herald added a subscriber: imp. · View Herald TranscriptNov 23 2020, 1:51 PM

trasz requested review of this revision.Nov 23 2020, 1:51 PM

Harbormaster completed remote builds in B34979: Diff 79896.Nov 23 2020, 1:52 PM

trasz added a subscriber: NetApp.Nov 23 2020, 1:52 PM

trasz retitled this revision from Fix use after fr in msdosfs_rename() to Fix use after free in msdosfs_rename().Nov 23 2020, 2:06 PM

trasz edited the summary of this revision. (Show Details)

This is not a proper fix. ip is invalid the very moment fvp is unlocked.

In D27337#610604, @kib wrote:

This is not a proper fix. ip is invalid the very moment fvp is unlocked.

"ip" as the pointer (later used to compare it with "xp"), or vnode data it points to?

In D27337#610846, @trasz wrote:

In D27337#610604, @kib wrote:

This is not a proper fix. ip is invalid the very moment fvp is unlocked.

"ip" as the pointer (later used to compare it with "xp"), or vnode data it points to?

Well, is a pointer to freed memory valid ? This is somewhat scholastic question, and I believe that our kernel depends on pointer value itself not becoming a poisoned value after free.
But the data it points to is invalid, and your patch still accesses the potentially freed or reused memory.

In D27337#613495, @kib wrote:

In D27337#610846, @trasz wrote:

In D27337#610604, @kib wrote:

This is not a proper fix. ip is invalid the very moment fvp is unlocked.

"ip" as the pointer (later used to compare it with "xp"), or vnode data it points to?

Well, is a pointer to freed memory valid ? This is somewhat scholastic question, and I believe that our kernel depends on pointer value itself not becoming a poisoned value after free.
But the data it points to is invalid, and your patch still accesses the potentially freed or reused memory.

Can you show me where? From my reading of the code, if doingdirectory != 0, the ip must be valid (equal to xp), due to checks at 1139-1141.

In D27337#614568, @trasz wrote:

In D27337#613495, @kib wrote:

In D27337#610846, @trasz wrote:

In D27337#610604, @kib wrote:

This is not a proper fix. ip is invalid the very moment fvp is unlocked.

"ip" as the pointer (later used to compare it with "xp"), or vnode data it points to?

Well, is a pointer to freed memory valid ? This is somewhat scholastic question, and I believe that our kernel depends on pointer value itself not becoming a poisoned value after free.
But the data it points to is invalid, and your patch still accesses the potentially freed or reused memory.

Can you show me where? From my reading of the code, if doingdirectory != 0, the ip must be valid (equal to xp), due to checks at 1139-1141.

It is valid until fvp is unlocked, after which it is invalid.

Make sure the ip is valid.

Harbormaster completed remote builds in B35343: Diff 80580.Dec 11 2020, 1:08 PM

Ah, right. What do you think about the new patch? I've tried avoiding the relocking, but the code flow there is somewhat complicated and I'd probably introduce more bugs.

I do not think this patch is good either. You should clear DE_RENAME right before unlock.

Which unlock? The gotos at 1048, 1057, 1062, 1076, 1080, 1085, and 1089 all get here without the lock held.

In D27337#621454, @trasz wrote:

Which unlock? The gotos at 1048, 1057, 1062, 1076, 1080, 1085, and 1089 all get here without the lock held.

Any unlock.

If fact, it is interesting to look at uses of DE_RENAME. Idea is to prevent directory removal and rename going in parallel. But, wouldn't vnode locking have the same effect? Why do we need to prevent this for the renamed vnode?

trasz mentioned this in D31366: msdosfs: drop DE_RENAME.Jul 31 2021, 6:12 PM

https://reviews.freebsd.org/D31366