Page MenuHomeFreeBSD

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

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

Details

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

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 created this revision.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.
delphij accepted this revision.Dec 13 2018, 12:53 AM
delphij added a subscriber: jpaetzel.

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
markj added a comment.Dec 13 2018, 1:02 AM

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.

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.

markj added a comment.Fri, Jan 4, 6:42 PM

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?

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.

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.

markj added a comment.Fri, Jan 4, 6:58 PM

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.

Yeah, it's simple enough that I'm not too worried.

This revision was automatically updated to reflect the committed changes.