Page MenuHomeFreeBSD

sound: Revalidate after reacquiring locks when resizing buffers
ClosedPublic

Authored by markj on Fri, Oct 9, 9:14 PM.
Tags
None
Referenced Files
F176401697: D60554.id189874.diff
Sun, Oct 18, 2:58 PM
F176383928: D60554.id189732.diff
Sun, Oct 18, 10:36 AM
F176332114: D60554.id189595.diff
Sun, Oct 18, 12:04 AM
Unknown Object (File)
Sat, Oct 17, 7:00 AM
Unknown Object (File)
Sat, Oct 17, 1:06 AM
Unknown Object (File)
Fri, Oct 16, 9:13 PM
Unknown Object (File)
Fri, Oct 16, 1:23 PM
Unknown Object (File)
Fri, Oct 16, 12:57 PM
Subscribers

Details

Summary

sndbuf_resize() and sndbuf_remalloc() drop the channel lock in order to
allocate the new buffer(s) with M_WAITOK. Before doing that they verify
that the buffer is not mapped, but this could change while the lock is
dropped, as there is no higher-level synchronization which prevents
buffer resize (e.g., via the SNDCTL_DSP_SETBLKSIZE ioctl) from happening
concurrently with mmap. Thus, these checks are insufficient.

Modify sndbuf_resize() and sndbuf_remalloc() to revalidate after
reacquiring the lock. Extend them to check for a couple of channel
flags, mirroring the check in chn_resizebuf(). Modify chn_resizebuf()
to revalidate state after reacquiring the channel lock.

We could also allocate new buffers with M_NOWAIT and so avoid dropping
the lock. Then, I think we could go even further and invoke
CHANNEL_SETBLOCKSIZE and CHANNEL_SETFRAGMENTS with the channel lock
held. But really it would be better to ensure that dsp operations are
serialized; there is no reason for mmap and chn_resizebuf() to be able
to run in parallel, we should not be relying on the channel lock for
this. So, let's keep a more minimal approach here.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 77929
Build 74812: arc lint + arc unit

Event Timeline

markj requested review of this revision.Fri, Oct 9, 9:14 PM

Fix an error path in sndbuf_remalloc().

Add a regression test. This doesn't exercise the race, rather it just
verifies that userspace cannot resize a mapped buffer.

christos added inline comments.
tests/sys/sound/mmap.c
172 ↗(On Diff #189732)

Should the rest of the test cases call munmap() and close() like that too?

This revision is now accepted and ready to land.Wed, Oct 14, 11:25 AM
tests/sys/sound/mmap.c
172 ↗(On Diff #189732)

Well, here it's part of the test. In general, yes, I think it's a good idea to explicitly release resources and check for errors at the end of a test, but that's not what this is for.