Add bounds checking to the tws(4) passthrough ioctl handler.
ClosedPublic
Actions

Authored by markj on Dec 12 2018, 11:40 PM.

Details

Reviewers

delphij
emaste

Commits

rS342787: Add a bounds check to the tws(4) passthrough ioctl handler.

Summary

tws_passthru() does a copyin of a user-specified request without
validating the length. This can only be exploited by root, however.

Test Plan

None. I have no way of testing this.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 21527
Build 20838: arc lint + arc unit

Event Timeline

markj created this revision.Dec 12 2018, 11:40 PM

Harbormaster completed remote builds in B21527: Diff 51940.Dec 12 2018, 11:40 PM

markj edited the test plan for this revision. (Show Details)Dec 12 2018, 11:41 PM

markj added reviewers: delphij, emaste.

LGTM (the unlocked use of sc->ioctl_data_mem looks worrisome to me, but the proposed change won't worsen the situation). Do you have a chance to test this on real hardware? (@jpaetzel do you know someone who may be able to help with that?).

This revision is now accepted and ready to land.Dec 13 2018, 12:53 AM

In D18536#395145, @delphij wrote:

LGTM (the unlocked use of sc->ioctl_data_mem looks worrisome to me, but the proposed change won't worsen the situation).

I was wondering about that too. AFAIK there is no mechanism at the upper layers to serialize calls to the ioctl handler, so two threads can race here.

Do you have a chance to test this on real hardware? (@jpaetzel do you know someone who may be able to help with that?).

I don't own any hardware driven by tws. Any help on that front would be much appreciated.

In D18536#395145, @delphij wrote:

LGTM (the unlocked use of sc->ioctl_data_mem looks worrisome to me, but the proposed change won't worsen the situation). Do you have a chance to test this on real hardware? (@jpaetzel do you know someone who may be able to help with that?).

My last 9750 died a while ago. I'll ping Austin @ ix to see if he can rig up a system for us to test with.

In D18536#395272, @jpaetzel wrote:

In D18536#395145, @delphij wrote:

LGTM (the unlocked use of sc->ioctl_data_mem looks worrisome to me, but the proposed change won't worsen the situation). Do you have a chance to test this on real hardware? (@jpaetzel do you know someone who may be able to help with that?).

My last 9750 died a while ago. I'll ping Austin @ ix to see if he can rig up a system for us to test with.

Ping?

In D18536#400013, @markj wrote:

In D18536#395272, @jpaetzel wrote:

In D18536#395145, @delphij wrote:

LGTM (the unlocked use of sc->ioctl_data_mem looks worrisome to me, but the proposed change won't worsen the situation). Do you have a chance to test this on real hardware? (@jpaetzel do you know someone who may be able to help with that?).

My last 9750 died a while ago. I'll ping Austin @ ix to see if he can rig up a system for us to test with.

Ping?

I haven't been deemed worthy of a reply, so I guess that's a no.

In D18536#400014, @jpaetzel wrote:

In D18536#400013, @markj wrote:

In D18536#395272, @jpaetzel wrote:

In D18536#395145, @delphij wrote:

LGTM (the unlocked use of sc->ioctl_data_mem looks worrisome to me, but the proposed change won't worsen the situation). Do you have a chance to test this on real hardware? (@jpaetzel do you know someone who may be able to help with that?).

My last 9750 died a while ago. I'll ping Austin @ ix to see if he can rig up a system for us to test with.

Ping?

I haven't been deemed worthy of a reply, so I guess that's a no.

Thanks, let's land the change then.

In D18536#400015, @delphij wrote:

In D18536#400014, @jpaetzel wrote:

In D18536#400013, @markj wrote:

In D18536#395272, @jpaetzel wrote:

In D18536#395145, @delphij wrote:

LGTM (the unlocked use of sc->ioctl_data_mem looks worrisome to me, but the proposed change won't worsen the situation). Do you have a chance to test this on real hardware? (@jpaetzel do you know someone who may be able to help with that?).

My last 9750 died a while ago. I'll ping Austin @ ix to see if he can rig up a system for us to test with.

Ping?

I haven't been deemed worthy of a reply, so I guess that's a no.

Thanks, let's land the change then.