sound: Refactor dsp_mmap_single() #30
emaste
kib
markj
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "christos/mmap"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
sound: Refactor dsp_mmap_single()to WIP: sound: Refactor dsp_mmap_single()@ -1964,2 +1942,2 @@(rdch != NULL && (rdch->flags & CHN_F_MMAP_INVALID))) {dsp_unlock_chans(priv, FREAD | FWRITE);CHN_LOCK(c);if (c->flags & CHN_F_MMAP_INVALID || c->bufsoft->allocsize < size) {What if CHN_F_MMAP_INVALID is set during the region where c is unlocked?
IMO you should re-arrange the actions, not assigning to '*object' until the second CHN_LOCK(), and checking the conditions then. The pre-check for these conditions is useful still, I believe.
166fdfbf20dc63cdb6dcdc63cdb6dc3f8772bbbfThe solution to the overflow is to silently ignore the user-supplied offset? Isn't that likely to break something?
@markj wrote in #30 (comment):
It isn't the solution to the overflow per-se, more of a side effect. I explain that in the commit message, but I do not see why we should care about the offset, or how it can be useful, or how it is even currently useful, given how sound(4)'s buffer allocations work. That is, for each
pcm_channel(assigned to one file descriptor) we allocate a pairsnd_dbufs, one consumed by the device driver, one by userland. What mmap(2) does is simply map pages on top of that allocated buffer (the userland-facing one). I am skeptical about whether this is correct design, but it's how sound(4) has been doing it forever.The reason I'm saying the offset is essentially useless, even with the current implementation, is because
sndbuf_getbufofs()itself, as the commit message, as well as the bug submitter, suggests, truncates thevm_ooffset_tvalue, and also has aKASSERTto make sure the offset is always smaller than the buffer size. In other words, the offset is by design meant to be within the buffer's bounds. So what benefit is it? I don't see why someone would intentionally map their buffer just at a higher offset.The OSS manual itself [1] does not seem to include information about how the offset parameter should be handled.
[1] http://manuals.opensound.com/developer/mmap.html
Yes, the intent is to be able to map starting at some (page-aligned) offset in the buffer. I don't know what benefit that has either, but Chesterson's Fence seems to apply here. At least, I wouldn't be comfortable shipping this as a security patch if we don't know whether it might break something. Why not just directly fix the check to handle overflow? Then any needed cleanup can come later.
@markj wrote in #30 (comment):
The current implementation is wrong though. We are passing a
vm_ooffset_ttosndbuf_getbufofs()which expects auint32_t, and because of the truncation combined with howsndbuf_getbufofs()actually works, we're changing the semantics of what the offset parameter is, and we use it as a buffer index instead, which from an application perspective, is useless. Additionally, I cannot see how this could cause any issues, since these buffers are assigned to specific channels, and the offset functions as an index inside those per-channel buffers. Each mmap(2) call eventually returns the buffer for that specific channel, almost always at index 0 (I haven't seen an OSS mmap(2) call with a non-zero offset argument so far), or at some index inside that buffer. For this reason, we cannot end up with overlapping mappings, which I suppose would be an issue you'd be worried about.@christos wrote in #30 (comment):
What do you mean by "changing the semantics"? It's up to dsp_mmap_single() to define the semantics in the first place, and it's treating
offsetas an offset into the channel buffer selected byprot. Unless there are channel buffers larger than 4GB, there is no truncation happening.I'm worried about breaking applications with a security patch, that's all. If you're convinced that no applications are passing a non-zero offset, then fine, dsp_mmap_single() can be reworked, but to start we should land a minimal patch which fixes the overflow bug.
3f8772bbbf5812ee03ab@markj wrote in #30 (comment):
Updated the diff.
WIP: sound: Refactor dsp_mmap_single()to sound: Refactor dsp_mmap_single()Mark's comments are addressed. This should be good to go.
@ -1921,2 +1902,4 @@struct pcm_channel *c;int err;if (*offset >= (*offset + size))Extra ()
5812ee03abbcb8de400a@ -1955,2 +1936,2 @@(rdch != NULL && (rdch->flags & CHN_F_MMAP_INVALID))) {dsp_unlock_chans(priv, FREAD | FWRITE);CHN_LOCK(c);if (c->flags & CHN_F_MMAP_INVALID ||(c->flags & CHN_F_MMAP_INVALID) != 0
This is an issue in many places throughout sound(4). I was thinking of fixing all these in a patch in one go instead, no?
@ -1970,0 +1948,4 @@if (*object != NULL) {CHN_LOCK(c);if (c->flags & CHN_F_MMAP_INVALID) {() != 0
@ -1967,3 +1944,1 @@dsp_unlock_chans(priv, FREAD | FWRITE);*object = vm_pager_allocate(OBJT_DEVICE, i_dev,size, nprot, *offset, curthread->td_ucred);CHN_UNLOCK(c);Does the invariant c->bufsoft->allocsize < *offset + size stays true after the channel unlock?
Theoretically, allocsize could change if the buffer gets resized during that window. I dont think such a scenario is likely at all, but I could add a check once we pick up the lock again for robustness.
Then it raises a more generic question, what would happen when (not even if) the buffer is resized after the object is allocated. Would it provide an access to some random memory after the buffer?
I was thinking the exact same thing now. Because the mappings are essentially a pointer to the sound(4) buffer, then yes, if we re-allocate it, and we end up in different addresses, then the object allocation would still point to the old addresses.
sys/dev/sound/pcm/buffer.cdoesn't seem to reallocate memory if the requested size is the same as the current one. I suppose we could cacheallocsizewhen before we drop the lock, and test that againstc->allocsizeafter we pick up the lock back. If the sizes are different, then we deallocate the object and fail. What do you think?How is the reallocation initiated? I would consider disabling it if there is at least one mapping.
There seem to be 2 reallocation functions, but one that actually remallocs
c->bufsoft->buf, which is where the mmap address comes from, and that's insndbuf_remalloc(). This one is called only fromchn_resizebuf(), but the function fails at the very beginning ifCHN_F_MMAPis set.In one of my patches here I move the
CHN_F_MMAPsetting after everything has succeeded, sochn_resizebuf()will be able to continue. I could bring back the check before we unlock, and instead of setting the flag when everything succeeds, I can just clear it we fail.The other function,
sndbuf_resize()does changeallocsizeif the size is different than the current one, but doesn't remallocbuf, it just mallocstmpbuf. I don't fully understand this, but this function is called only from the device drivers, so it actually affectsc->bufhard, notc->bufsoft, so we can ignore it.bcb8de400ae610965bdfCould there be more than one mapping for the channel? If yes, you cannot use a flag to track it.
Thanks, I will stage the overflow check for the next secteam release. Please avoid landing patches in this area until then, as that will make life easier when we push patches to main/stable/releng branches, and since there will be another patch on top of this one in the secteam release (I will post a patch for review soon).
In general we should avoid publicly posting patches which fix severe security bugs. My workflow for this is to create a phab revision with
git arc create -d, then in the webui, limit visibility to subscribers only, then public the review. This is not perfect but I don't think there's a better way to handle it, short of emailing patches around.@kib wrote in #30 (comment):
Each FD gets up to 2 channels, one recording and one for playback, depending on what's supported and how the FD was opened. So an mmap(2) call can map only one channel, no more mappings can happen.
One scenario I haven't investigated is: if for whatever reason, the application calls mmap(2) more than once, without the equivalent munmap()s before, will the mmap(2) handler ignore subsequent calls, or is it up to sound(4) to ignore it, for example by failing if
CHN_F_MMAPis already set?For each mmap(2) call from userspace, there is a corresponding call to d_mmap_single() at the driver level.
There is no problem with multiple mappings, since all mappings happen inside the same channel buffer anyway. We don't need to do anything more.
e610965bdfb3d631dd4e@markj Rebased to include the latest mmap patches. Do you have any comment, particularly for "sound: Handle CHN_F_MMAP_INVALID after cdev_pager_allocate()"?
Bump.
b3d631dd4ee6eaf128f6@ -2009,0 +1986,4 @@* PROT_WRITE|PROT_READ together select the output buffer.*/c = ((nprot & PROT_WRITE) != 0) ? priv->wrch : priv->rdch;if (c == NULL)Missing PCM_GIANT_EXIT in the return path.
@ -2035,0 +2011,4 @@if (c->flags & CHN_F_MMAP_INVALID) {err = EINVAL;c->flags &= ~CHN_F_MMAP;free(handle, M_DEVBUF);I believe you need to free the object as well, i.e.,
vm_object_deallocate(*object). Please check this.I believe there is no risk of the buffer size changing, the CHN_F_MMAP flag should block that.
vm_object_deallocate()causes a double-free panic, justfree(handle)is enough.I am skeptical. Where did you call vm_object_deallocate()? How did you trigger the race, and where did the kernel panic?
It panicked when it was put both before and after the
free().Before
free():After
free():Eh, so that's because dsp_dev_pager_dtor() is running. You need to remove the free(handle), not the vm_object_deallocate().
e6eaf128f6e0794c09a8e0794c09a861fe5d1299@ -2035,0 +2013,4 @@if (c->flags & CHN_F_MMAP_INVALID) {err = EINVAL;c->flags &= ~CHN_F_MMAP;vm_object_deallocate(*object);I believe it is not safe to deallocate the object while holding the channel lock, vm_object_deallocate() may sleep in general.
61fe5d1299646c27f175646c27f175730eaf4664