Fix use after free in msdosfs_rename()

[trasz]

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.

[kib]

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

[trasz]

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?

[kib]

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.

[trasz]

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.

[kib]

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.

[trasz]

Make sure the ip is valid.

[trasz]

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.

[kib]

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

[trasz]

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

[kib]

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.

[kib]

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]

https://reviews.freebsd.org/D31366