vfs: fix memory leak on lookup with fds with ioctl caps

[mjg]

Summary

Add WANTIOCTLCAPS so that the one caller which looks at them gets to keep the content. This is a minimal patch to fix the problem, subject to rework later.

[mjg]

All the extra checks for other lookups keep irking me. I'm going to do it better with forcing WANTFILECAPS consumer to free caps (if any) on its own on namei failure.

[mjg]

• make the ioctl caps-wanting caller responsible for always freeing them, even on namei failure

note rename also looks at caps, but happens to not look at ioctl.

[mjg]

• missing filecaps_free in NDREINIT

[markj]

Thanks for working on this. I have a couple of comments:

• I think WANTIOCTLCAPS should just be WANTCAPS. kern_openat() doesn't know or care about ioctls, so the need to specify "ioctl caps" is a layering violation.
• Can we assert in NDFREE() that ni_filecaps is clear?

[mjg]

In D34667#785494, @markj wrote:

• I think WANTIOCTLCAPS should just be WANTCAPS. kern_openat() doesn't know or care about ioctls, so the need to specify "ioctl caps" is a layering violation.

well, rename looks at caps, but never takes a look at ioctl. with the proposed change it would also have to add the flag and make sure to free stuff later. I can't stress enough that explicit mention of ioctls, which is of no significance to almost anyone, allows to *ignore* presence of ioctls later on, not requiring non-kern-openat consumers to do anything extra.

• Can we assert in NDFREE() that ni_filecaps is clear?

no, because currently NDFREE can be called at an arbitrary time and only to take care of part of the collected result. the current contract is incredibly loose.

[markj]

In D34667#785496, @mjg wrote:

In D34667#785494, @markj wrote:

• I think WANTIOCTLCAPS should just be WANTCAPS. kern_openat() doesn't know or care about ioctls, so the need to specify "ioctl caps" is a layering violation.

well, rename looks at caps, but never takes a look at ioctl. with the proposed change it would also have to add the flag and make sure to free stuff later. I can't stress enough that explicit mention of ioctls, which is of no significance to almost anyone, allows to *ignore* presence of ioctls later on, not requiring non-kern-openat consumers to do anything extra.

To be clear, I'm just suggesting a name change. If some other dynamically allocated capability limit is added to struct filecaps then the names WANTIOCTLCAPS and NDFREE_IOCTLCAPS() will be wrong and misleading. They should just be WANTCAPS and NDFREE_CAPS() with no other modification; consumers don't know or care about ioctl limits, so any such details should be hidden from them.

sys/kern/kern_descrip.c

1809

This can and should be local to kern_descrip.c.

[mjg]

• make filecaps_free_ioctl static

[mjg]

In D34667#785509, @markj wrote:

In D34667#785496, @mjg wrote:

In D34667#785494, @markj wrote:

• I think WANTIOCTLCAPS should just be WANTCAPS. kern_openat() doesn't know or care about ioctls, so the need to specify "ioctl caps" is a layering violation.

well, rename looks at caps, but never takes a look at ioctl. with the proposed change it would also have to add the flag and make sure to free stuff later. I can't stress enough that explicit mention of ioctls, which is of no significance to almost anyone, allows to *ignore* presence of ioctls later on, not requiring non-kern-openat consumers to do anything extra.

To be clear, I'm just suggesting a name change. If some other dynamically allocated capability limit is added to struct filecaps then the names WANTIOCTLCAPS and NDFREE_IOCTLCAPS() will be wrong and misleading. They should just be WANTCAPS and NDFREE_CAPS() with no other modification; consumers don't know or care about ioctl limits, so any such details should be hidden from them.

But with your proposal rename has to add the flag, otherwise it has no "right" to look at the caps stored in nameidata itself -- it did not ask for caps to be there. How about WANTFULLCAPS instead?

[markj]

In D34667#785518, @mjg wrote:

In D34667#785509, @markj wrote:

In D34667#785496, @mjg wrote:

In D34667#785494, @markj wrote:

• I think WANTIOCTLCAPS should just be WANTCAPS. kern_openat() doesn't know or care about ioctls, so the need to specify "ioctl caps" is a layering violation.

well, rename looks at caps, but never takes a look at ioctl. with the proposed change it would also have to add the flag and make sure to free stuff later. I can't stress enough that explicit mention of ioctls, which is of no significance to almost anyone, allows to *ignore* presence of ioctls later on, not requiring non-kern-openat consumers to do anything extra.

To be clear, I'm just suggesting a name change. If some other dynamically allocated capability limit is added to struct filecaps then the names WANTIOCTLCAPS and NDFREE_IOCTLCAPS() will be wrong and misleading. They should just be WANTCAPS and NDFREE_CAPS() with no other modification; consumers don't know or care about ioctl limits, so any such details should be hidden from them.

But with your proposal rename has to add the flag, otherwise it has no "right" to look at the caps stored in nameidata itself -- it did not ask for caps to be there. How about WANTFULLCAPS instead?

I see, ugh. Well, that's ok, we are unconditionally copying the ioctl limits today anyway so there is no performance impact. We should really have a filecaps_check() function to wrap the cap_check() call as well.

But ok, I do not object to the current patch then. Better to fix the leak minimally I suppose.