modstat: avoid copying from freed memory

[brooks]

Summary

If a module is unloaded after the module is looked up in kern_modstat,
but before one of the copyout's occurs, then the memory being copied
might be freed and reused leading to an information disclosure.

The LLM was particularly concerned about a TOCTOU issue in the length
of the name, but it seems likely that there are issues with all the
copyout's. As such, hold the shared lock throughout.

Reported by: llm-postcr
Fixes: 9b3851e9e3b1 ("- Lock down the ``module'' structure...")

[brooks]

Using a macro for return isn't the most idiomatic, but I didn't want the churn of adding braces to every check and it feels more inline with possibly adding EXTERR support in the future.

[markj]

Thanks for fixing this.

In D61398#1407306, @brooks wrote:

Using a macro for return isn't the most idiomatic, but I didn't want the churn of adding braces to every check and it feels more inline with possibly adding EXTERR support in the future.

Can't you just sprinkle goto out with:

out:
    MOD_SUNLOCK;
    return (error);

?

That way, you only need to add braces in one place, to set error = EINVAL.

I think freebsd32_modstat() has the same problem.

[brooks]

use goto out pattern

[markj]

sys/kern/kern_module.c

540–541

This function has the same problems I think.

[brooks]

• Handle freebsd32_modstat
• Use the same pattern for the lookup failure

[markj]

I think this is okay. It might not be a good idea to hold this global lock across copyin/copyout. If someone runs kldstat or similar and the kernel needs to fault in the user output buffer, then we could block under memory pressure with that lock held, which could further hurt responsiveness of the system.

[brooks]

In D61398#1407706, @markj wrote:

I think this is okay. It might not be a good idea to hold this global lock across copyin/copyout. If someone runs kldstat or similar and the kernel needs to fault in the user output buffer, then we could block under memory pressure with that lock held, which could further hurt responsiveness of the system.

It's not ideal, but I think I'm OK with turning a UAF disclosure bug into voluntary poor performance. The better answer is probably to do something like:

1. lock, do the lookup, drop the lock
2. do the copyin
3. malloc an appropriate struct modstat*
4. lock, lookup, fill the struct, drop the lock
5. do the copyout