From nobody Thu Nov 27 00:33:34 2025 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4dGy8Z4DRwz6HtCJ for ; Thu, 27 Nov 2025 00:33:34 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R12" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4dGy8Z3ZMzz43VM for ; Thu, 27 Nov 2025 00:33:34 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1764203614; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=lzQ5n1kwq+mFr6MfTAvbRzCR2Y75dR8CGx/aiwzA2y4=; b=eEXlIOE/OiJNAAEDtX2iRlMia2b1X2+UOStUBiHasQwo7hq6WY720CocsQd8Qo4lfSI4X7 hScba3nMePEkbDujbJIN2kkztjg7LR+J4EBxpm4d+UpXI0RnbBxzZRGTmJfyGgBtMSAe7j ac8SkS2ArBLBBGq0yxpkjT4/Dt5fsKaK8NNuVMAnixhPct6uDVOo0gHAYFV70A+iH7GqGr TOKmIHy8jfVJh9k+atX1WNqu32koplJLFdkhfKzuZS60+ueJsFxl975O+/h48fw3SxUg9C EJ5qEtVZw+rm5OFcsRXAskR7jKOneSOFLYPJeLoy6dl53S3HYuPyT4/UB7kbCw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1764203614; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=lzQ5n1kwq+mFr6MfTAvbRzCR2Y75dR8CGx/aiwzA2y4=; b=yRsaqp9Y/4iCCLeOMvEW5WdYYoLMRuyoHVcsmAbuGWA1B0h33hmQdR3pe15GViGBzxdQh4 /H1bIa58qlIG2JrZKm3kgOxzD7jgqCtAB6sTtwBOD3HhmPPlZXjur1T5M1sQNO58Fb/gFg O+U2RnVhmTJxRrcIzqppVYnmxe4I8WDxPjIwwEtc4wVRnUzkoj7GBQA43hrq8ob590RINp yq2flo1r263zwa0zE05eYZ1jqdgXWgeQj3cy1IOgilHzCfQl2XUvtVKfX+xRmQ8Rwg7hS0 /m2AEn//qWaB7iS9R3dyCYr9fBQkHekTWT+2CA528g3qzRIfuY/OdZQoKiy45g== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1764203614; a=rsa-sha256; cv=none; b=oSMVwq4sal09Vu3jnjNzxhIOt7QNphTfXMpWc2W++eyQtvM48hsU7fkb9/fw1SBSd1PG6I jCTrY6wOX3gUrn6uGfdmdO/Ul1f08plRTAYY7ZHeDvq9RtEJ5708tOEfMZiB3ZnP7W2aW0 XIDbnkgGSvua9RwrWI4tdpAs3N/dGRc/yAnLzHaFUf743eitSx2FsPfmtsNcNlVtkHhoJZ xt+Sg0ln/kAcRjmDhr9ZBestOl+V7hErsE/U45932V6fdVqQtt6PMmVG9+iGBFZxRjQQfA 9xn1i5ov6JThsxUcvNJqrqWwBLR3cIJmnRtkvGhIWlMv2uea2v3PGzlrRWl/Cg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) by mxrelay.nyi.freebsd.org (Postfix) with ESMTP id 4dGy8Z2ySpz18Jf for ; Thu, 27 Nov 2025 00:33:34 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from git (uid 1279) (envelope-from git@FreeBSD.org) id f2b6 by gitrepo.freebsd.org (DragonFly Mail Agent v0.13+ on gitrepo.freebsd.org); Thu, 27 Nov 2025 00:33:34 +0000 To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Christos Margiolis Subject: git: 068b20e200fb - stable/15 - sound: Fix KASSERT panics in chn_read() and chn_write() List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: christos X-Git-Repository: src X-Git-Refname: refs/heads/stable/15 X-Git-Reftype: branch X-Git-Commit: 068b20e200fbd9db5dbe102f9e0715c121b86494 Auto-Submitted: auto-generated Date: Thu, 27 Nov 2025 00:33:34 +0000 Message-Id: <69279c5e.f2b6.30064da5@gitrepo.freebsd.org> The branch stable/15 has been updated by christos: URL: https://cgit.FreeBSD.org/src/commit/?id=068b20e200fbd9db5dbe102f9e0715c121b86494 commit 068b20e200fbd9db5dbe102f9e0715c121b86494 Author: Christos Margiolis AuthorDate: 2025-11-20 15:23:09 +0000 Commit: Christos Margiolis CommitDate: 2025-11-27 00:33:08 +0000 sound: Fix KASSERT panics in chn_read() and chn_write() INVARIANTS kernels may trigger a KASSERT panic from sndbuf_acquire(), when fuzzing write(2) using stress2, because of a race in chn_write(). In the case of chn_write(), what sndbuf_acquire() does is extend the ready-to-read area of the buffer by a specified amount of bytes. The KASSERT in question makes sure the number of bytes we want to extend the ready area by, is less than or equal to the number of free bytes in the buffer. This makes sense, because we cannot extend the ready area to something larger than what is available (i.e., free) in the first place. What chn_write() currently does for every write is; calculate the appropriate write size, let's say X, unlock the channel, uiomove() X bytes to the channel's buffer, lock the channel, and call sndbuf_acquire() to extend the ready area by X bytes. The problem with this approach, however, is the following. Suppose an empty channel buffer with a length of 1024 bytes, and 2 threads, (A) and (B), where (B) is a higher-priority one. Suppose thread (A) wants to write 1024 bytes. It unlocks the channel and uiomove()s 1024 bytes to the channel buffer. At the same time, thread (B) picks up the lock, and because it is higher priority, it keeps dominating the lock for a few iterations. By the time thread (A) picks up the lock again, it tries to call sndbuf_acquire() with a size of 1024 bytes, which was calculated before it performed the uiomove(). In this case, there is a very high chance that the buffer will not be empty, that is, have a free area of 1024 bytes, as was the case when thread (A) started executing, and so the KASSERT will trigger a panic because the condition (bytes <= free) is not met. Another scenario that can trigger a panic is the following: suppose a buffer with a size of 4 bytes, and two threads: (A) and (B). In the first iteration, thread (A) wants to write 2 bytes, while the buffer is empty, BUT the pointer (sndbuf_getfreeptr()) is at the end (i.e., buf[3]). In the first iteration of the loop, because of the way we calculate t, we'll end up writing only 1 byte, so after sz -= t, sz will be 1, and so we'll need one more iteration in the inner loop, to write the remaining 1 byte. Now we're at the end of the first loop, thread (A) unlocks the channel, it has written 1 byte, it needs to write 1 more, and the buffer is left with 3 empty slots. Now thread (B) picks up the lock, and it wants to write 3 (or more) bytes. Eventually it writes the 3 bytes, and it leaves the buffer with 0 free slots. By the time thread (A) picks up the lock again, and continues with the second iteration of the inner loop, it will try to write the last byte, but sndbuf_acquire() will panic because there is no free space anymore. To fix this, get rid of the inner loop and calculate the write size on each iteration. Also, call sndbuf_acquire() before unlocking the channel. In the scenarios explained above, we'll end up entering the chn_sleep() case. Modify it as well, so that we do not kill the channel if we need to sleep more. Do the same for chn_read() to avoid possible similar panics from sndbuf_dispose(). Reported by: pho Tested by: christos, pho Sponsored by: The FreeBSD Foundation MFC after: 1 week Reviewed by: pho, kib Differential Revision: https://reviews.freebsd.org/D53666 (cherry picked from commit 253b98f749cf93a9a682f46925c43cbbd04e1110) --- sys/dev/sound/pcm/channel.c | 70 ++++++++++++++------------------------------- 1 file changed, 22 insertions(+), 48 deletions(-) diff --git a/sys/dev/sound/pcm/channel.c b/sys/dev/sound/pcm/channel.c index f29a819ce0ae..e92181d74e19 100644 --- a/sys/dev/sound/pcm/channel.c +++ b/sys/dev/sound/pcm/channel.c @@ -438,7 +438,7 @@ chn_write(struct pcm_channel *c, struct uio *buf) { struct snd_dbuf *bs = c->bufsoft; void *off; - int ret, timeout, sz, t, p; + int ret, timeout, sz, p; CHN_LOCKASSERT(c); @@ -446,24 +446,17 @@ chn_write(struct pcm_channel *c, struct uio *buf) timeout = chn_timeout * hz; while (ret == 0 && buf->uio_resid > 0) { + p = sndbuf_getfreeptr(bs); sz = min(buf->uio_resid, sndbuf_getfree(bs)); + sz = min(sz, bs->bufsize - p); if (sz > 0) { - /* - * The following assumes that the free space in - * the buffer can never be less around the - * unlock-uiomove-lock sequence. - */ - while (ret == 0 && sz > 0) { - p = sndbuf_getfreeptr(bs); - t = min(sz, bs->bufsize - p); - off = sndbuf_getbufofs(bs, p); - CHN_UNLOCK(c); - ret = uiomove(off, t, buf); - CHN_LOCK(c); - sz -= t; - sndbuf_acquire(bs, NULL, t); - } - ret = 0; + off = sndbuf_getbufofs(bs, p); + sndbuf_acquire(bs, NULL, sz); + CHN_UNLOCK(c); + ret = uiomove(off, sz, buf); + CHN_LOCK(c); + if (ret != 0) + break; if (CHN_STOPPED(c) && !(c->flags & CHN_F_NOTRIGGER)) { ret = chn_start(c, 0); if (ret != 0) @@ -483,13 +476,7 @@ chn_write(struct pcm_channel *c, struct uio *buf) ret = EAGAIN; } else { ret = chn_sleep(c, timeout); - if (ret == EAGAIN) { - ret = EINVAL; - c->flags |= CHN_F_DEAD; - device_printf(c->dev, "%s(): %s: " - "play interrupt timeout, channel dead\n", - __func__, c->name); - } else if (ret == ERESTART || ret == EINTR) + if (ret == ERESTART || ret == EINTR) c->flags |= CHN_F_ABORTING; } } @@ -552,7 +539,7 @@ chn_read(struct pcm_channel *c, struct uio *buf) { struct snd_dbuf *bs = c->bufsoft; void *off; - int ret, timeout, sz, t, p; + int ret, timeout, sz, p; CHN_LOCKASSERT(c); @@ -568,35 +555,22 @@ chn_read(struct pcm_channel *c, struct uio *buf) timeout = chn_timeout * hz; while (ret == 0 && buf->uio_resid > 0) { + p = sndbuf_getreadyptr(bs); sz = min(buf->uio_resid, sndbuf_getready(bs)); + sz = min(sz, bs->bufsize - p); if (sz > 0) { - /* - * The following assumes that the free space in - * the buffer can never be less around the - * unlock-uiomove-lock sequence. - */ - while (ret == 0 && sz > 0) { - p = sndbuf_getreadyptr(bs); - t = min(sz, bs->bufsize - p); - off = sndbuf_getbufofs(bs, p); - CHN_UNLOCK(c); - ret = uiomove(off, t, buf); - CHN_LOCK(c); - sz -= t; - sndbuf_dispose(bs, NULL, t); - } - ret = 0; + off = sndbuf_getbufofs(bs, p); + sndbuf_dispose(bs, NULL, sz); + CHN_UNLOCK(c); + ret = uiomove(off, sz, buf); + CHN_LOCK(c); + if (ret != 0) + break; } else if (c->flags & (CHN_F_NBIO | CHN_F_NOTRIGGER)) ret = EAGAIN; else { ret = chn_sleep(c, timeout); - if (ret == EAGAIN) { - ret = EINVAL; - c->flags |= CHN_F_DEAD; - device_printf(c->dev, "%s(): %s: " - "record interrupt timeout, channel dead\n", - __func__, c->name); - } else if (ret == ERESTART || ret == EINTR) + if (ret == ERESTART || ret == EINTR) c->flags |= CHN_F_ABORTING; } }