Avoid leaking kernel pointers from msgctl(IPC_STAT).
ClosedPublic
Actions

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

Details

Reviewers

kib
emaste

Group Reviewers

manpages

Commits

rS363055: Apply the logic from r363051 to semctl(2) and __sem_base field.
rS363051: Avoid copying out kernel pointers from msgctl(IPC_STAT).

Summary

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

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 32214
Build 29710: arc lint + arc unit

Event Timeline

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

markj created this revision.

Harbormaster completed remote builds in B32213: Diff 74236.Jul 9 2020, 2:36 PM

markj added reviewers: kib, emaste.Jul 9 2020, 2:36 PM

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#566344, @markj 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.

In D25600#566353, @kib wrote:

In D25600#566344, @markj 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.