sys/openbsd: avoid using BIOCSETIF in causes "tun: read failed"

[blackgnezdo]

With luck this should prevent killing the executors with https://syzkaller.appspot.com/bug?extid=682dad6055938e287d19

[blackgnezdo]

@dvyukov could you recommend the incantation to tighten this check?

[a-nogikh]

Are you interested in how exactly you could check for the tap string in the third argument or something more general, e.g. if there are other ways to restrict it?

[blackgnezdo]

I just want this ioctl to not happen to tap interfaces because we rely on them working for packet injection.

[a-nogikh]

AFAIK there's currently to code that does something similar enough to this, but we could try to construct it from pieces.

1. Cast c.Args[2] to *prog.PointerArg. Let it be ptrArg.
2. Cast ptrArg.Res to *prog.GroupArg. Let it be groupArg.
3. Cast groupArg.Inner[0] to *prog.DataArg. Let it be dataArg.
4. Verify dataArg.Data() and patch it, if necessary. Like here:

syzkaller/sys/linux/init.go

Lines 241 to 253 in 32fcf98

	dataArg, ok := unionArg.Option.(*prog.DataArg)
	if !ok {
	return
	}
	if dataArg.Dir() == prog.DirOut {
	return
	}
	// Clear the first 16 bytes to prevent overcoming the limitation by squashing the struct.
	data := append([]byte{}, dataArg.Data()...)
	for i := 0; i < 16 && i < len(data); i++ {
	data[i] = 0
	}
	dataArg.SetData(data)

One this to watch out for are ANY types -- in that case, syzkaller has squahed the whole struct into one binary blob. We cannot really build a good tight sanity check in that case, so I think we can check for ANY and boldly patch it like you've already done it in this PR.

Here's an example of a check for ANY. You can do it for c.Args[2].

syzkaller/sys/linux/init_vusb.go

Lines 61 to 63 in 32fcf98

	if g.Target().ArgContainsAny(arg) {
	return
	}

[blackgnezdo]

Thanks for your guidance.

I implemented 1-4 but I'm not sure where to get a *prog.Gen to call .Target on.

Also not sure if there's an easy way to test this without pushing it to prod :)

[a-nogikh]

Also not sure if there's an easy way to test this without pushing it to prod :)

We do have quite a lot of tests for call sanitization in Linux: https://github.com/google/syzkaller/blob/master/sys/linux/init_test.go

I'm not sure where to get a *prog.Gen to call .Target on.

At the very least we could remember it in the struct arch {} -- the prog.Target pointer is passed to InitTarget.

[codecov]

Codecov Report

Attention: Patch coverage is 14.28571% with 6 lines in your changes missing coverage. Please review.

Project coverage is 60.3%. Comparing base (c699c2e) to head (0f0fa60).

Additional details and impacted files

Files	Coverage Δ
sys/openbsd/init.go	83.4% <14.3%> (-2.6%)	⬇️

... and 2 files with indirect coverage changes

[blackgnezdo]

I stared at https://syzkaller.appspot.com/bug?extid=682dad6055938e287d19 a bit more and came to the conclusion that I may not even be chasing the right problem. The last syz repro was from 2024/02/27.

If it is the right problem, then this PR effectively turns off this directive:

ioctl$BIOCSETIF(fd fd_bpf, cmd const[BIOCSETIF], arg ptr[in, ifreq_name])

If I wanted to do this I might as well remove this line completely, right?

[a-nogikh]

Yes, that should also work fine if syzkaller is unlikely to guess the BIOCSETIF const. I don't remember if we have the comparison argument substitution on OpenBSD - if yes, then it won't be too long until that happens.