file: Avoid a read-after-free of fd tables in sysctl handlers

[markj]

Summary

Some loops access the fd table of a different process, and drop the
filedesc lock while iterating, so they check the table's refcount.
However, we access the table before the first iteration, in order to get
the number of table entries, and this access may represent a
use-after-free.

Fix the problem by checking the refcount before we start iterating. I
don't see a nicer way to do it. BTW, I wonder why the FILEDESC_FOREACH
macros are exported by filedesc.h when they are only needed in
kern_descrip.c, especially since they use fdlastfile_single() and thus
don't assert that the table lock is held.

Reported by: pho

[mjg]

They are exported because everything else is exported. I have plans to convert the entire thing to bitset, so the fdlastfile_single routine is going away. I may end up moving stuff around to un-export things not needed outside.

[markj]

In D34575#783372, @mjg wrote:

They are exported because everything else is exported. I have plans to convert the entire thing to bitset, so the fdlastfile_single routine is going away. I may end up moving stuff around to un-export things not needed outside.

I just want to ensure that exported interfaces provide seatbelts to guard against misuse. The lack of a filedesc lock assertion is simply a bug, IMO.

sys/kern/kern_descrip.c

4199–4200

This check isn't needed, since the filedesc lock is not dropped in the loop.

[mjg]

In D34575#783393, @markj wrote:

In D34575#783372, @mjg wrote:

They are exported because everything else is exported. I have plans to convert the entire thing to bitset, so the fdlastfile_single routine is going away. I may end up moving stuff around to un-export things not needed outside.

I just want to ensure that exported interfaces provide seatbelts to guard against misuse. The lack of a filedesc lock assertion is simply a bug, IMO.

I would not say a bug. I did not want to add more variants of the macros, but I'm not going to oppose if you add the assert. As noted earlier, they are going to be reimlemented soon-ish anywah.

sys/kern/kern_descrip.c

4199–4200

that's an easy bug to introduce, so it should be here even if commented out with an explanation

[markj]

In D34575#783395, @mjg wrote:

In D34575#783393, @markj wrote:

In D34575#783372, @mjg wrote:

They are exported because everything else is exported. I have plans to convert the entire thing to bitset, so the fdlastfile_single routine is going away. I may end up moving stuff around to un-export things not needed outside.

I just want to ensure that exported interfaces provide seatbelts to guard against misuse. The lack of a filedesc lock assertion is simply a bug, IMO.

I would not say a bug. I did not want to add more variants of the macros, but I'm not going to oppose if you add the assert. As noted earlier, they are going to be reimlemented soon-ish anywah.

Well I don't think we can simply add an assert, there is at least one use which legitimately does not hold a filedesc lock. Hence my suggestion of making the macros private, at least. Or we can add a _SAFE variant.

[markj]

Remove an unneeded refcount check, reorganize existing checks.

[mjg]

In D34575#783413, @markj wrote:

In D34575#783395, @mjg wrote:

In D34575#783393, @markj wrote:

In D34575#783372, @mjg wrote:

They are exported because everything else is exported. I have plans to convert the entire thing to bitset, so the fdlastfile_single routine is going away. I may end up moving stuff around to un-export things not needed outside.

I just want to ensure that exported interfaces provide seatbelts to guard against misuse. The lack of a filedesc lock assertion is simply a bug, IMO.

I would not say a bug. I did not want to add more variants of the macros, but I'm not going to oppose if you add the assert. As noted earlier, they are going to be reimlemented soon-ish anywah.

Well I don't think we can simply add an assert, there is at least one use which legitimately does not hold a filedesc lock. Hence my suggestion of making the macros private, at least. Or we can add a _SAFE variant.

That's what I meant by "I did not want to add more variants of the macros". If you want to make them private now that's fine with me, just make the change, no need for a review.