Page MenuHomeFreeBSD

Avoid leaking kernel pointers from msgctl(IPC_STAT).
ClosedPublic

Authored by markj on Jul 9 2020, 2:35 PM.

Details

Summary

While this is harmless, these pointers are not useful to any user
code. Let's stop copying and documenting them.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Jul 9 2020, 2:35 PM
markj created this revision.

I do not object but also there is no much sense in this change. Both because we do not randomize KVA and because we export KVA layouts in many ways, some of which you added.

In D25600#566343, @kib wrote:

I do not object but also there is no much sense in this change. Both because we do not randomize KVA and because we export KVA layouts in many ways, some of which you added.

I understand, it is more about the intent: we have interfaces that export kernel pointers for debugging purposes, but here it is done because the same structure is used by both userland and the kernel, i.e., the interface is poorly designed. In that case I think it is reasonable to patch the leak, if only to avoid seeing the same reports again and again.

In that case I think it is reasonable to patch the leak, if only to avoid seeing the same reports again and again.

Agreed.

This revision is now accepted and ready to land.Jul 9 2020, 2:57 PM
In D25600#566343, @kib wrote:

I do not object but also there is no much sense in this change. Both because we do not randomize KVA and because we export KVA layouts in many ways, some of which you added.

I understand, it is more about the intent: we have interfaces that export kernel pointers for debugging purposes, but here it is done because the same structure is used by both userland and the kernel, i.e., the interface is poorly designed. In that case I think it is reasonable to patch the leak, if only to avoid seeing the same reports again and again.

Then perhaps add a comment at the place that nullifies the pointers explaining why.

BTW, a serious places that export kptrs do that not for debugging. The cases that I can remember immediately are kinfo_proc with pointers to struct proc/thread and wait channels.

In D25600#566353, @kib wrote:
In D25600#566343, @kib wrote:

I do not object but also there is no much sense in this change. Both because we do not randomize KVA and because we export KVA layouts in many ways, some of which you added.

I understand, it is more about the intent: we have interfaces that export kernel pointers for debugging purposes, but here it is done because the same structure is used by both userland and the kernel, i.e., the interface is poorly designed. In that case I think it is reasonable to patch the leak, if only to avoid seeing the same reports again and again.

Then perhaps add a comment at the place that nullifies the pointers explaining why.

BTW, a serious places that export kptrs do that not for debugging. The cases that I can remember immediately are kinfo_proc with pointers to struct proc/thread and wait channels.

Even there, it is not an accident. ps even has "-o paddr" for displaying such pointers. I've used it in the past to debug ptrace issues.

This revision now requires review to proceed.Jul 9 2020, 3:11 PM
This revision is now accepted and ready to land.Jul 9 2020, 3:18 PM