public inbox for stable@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 5.10.y] comedi: Fail COMEDI_INSNLIST ioctl if n_insns is too large
@ 2025-07-24 18:12 Ian Abbott
  2025-07-24 18:12 ` [PATCH 5.10.y] comedi: Fix some signed shift left operations Ian Abbott
                   ` (8 more replies)
  0 siblings, 9 replies; 21+ messages in thread
From: Ian Abbott @ 2025-07-24 18:12 UTC (permalink / raw)
  To: stable; +Cc: Ian Abbott, syzbot+d6995b62e5ac7d79557a, Greg Kroah-Hartman

[ Upstream commit 08ae4b20f5e82101d77326ecab9089e110f224cc ]

The handling of the `COMEDI_INSNLIST` ioctl allocates a kernel buffer to
hold the array of `struct comedi_insn`, getting the length from the
`n_insns` member of the `struct comedi_insnlist` supplied by the user.
The allocation will fail with a WARNING and a stack dump if it is too
large.

Avoid that by failing with an `-EINVAL` error if the supplied `n_insns`
value is unreasonable.

Define the limit on the `n_insns` value in the `MAX_INSNS` macro.  Set
this to the same value as `MAX_SAMPLES` (65536), which is the maximum
allowed sum of the values of the member `n` in the array of `struct
comedi_insn`, and sensible comedi instructions will have an `n` of at
least 1.

Reported-by: syzbot+d6995b62e5ac7d79557a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d6995b62e5ac7d79557a
Fixes: ed9eccbe8970 ("Staging: add comedi core")
Tested-by: Ian Abbott <abbotti@mev.co.uk>
Cc: stable@vger.kernel.org # 5.13+
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Link: https://lore.kernel.org/r/20250704120405.83028-1-abbotti@mev.co.uk
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/staging/comedi/comedi_fops.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 8f896e6208a8..5aa6a84d1fa6 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1584,6 +1584,16 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
 	return i;
 }
 
+#define MAX_INSNS   MAX_SAMPLES
+static int check_insnlist_len(struct comedi_device *dev, unsigned int n_insns)
+{
+	if (n_insns > MAX_INSNS) {
+		dev_dbg(dev->class_dev, "insnlist length too large\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
 /*
  * COMEDI_INSN ioctl
  * synchronous instruction
@@ -2234,6 +2244,9 @@ static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd,
 			rc = -EFAULT;
 			break;
 		}
+		rc = check_insnlist_len(dev, insnlist.n_insns);
+		if (rc)
+			break;
 		insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL);
 		if (!insns) {
 			rc = -ENOMEM;
@@ -3085,6 +3098,9 @@ static int compat_insnlist(struct file *file, unsigned long arg)
 	if (copy_from_user(&insnlist32, compat_ptr(arg), sizeof(insnlist32)))
 		return -EFAULT;
 
+	rc = check_insnlist_len(dev, insnlist32.n_insns);
+	if (rc)
+		return rc;
 	insns = kcalloc(insnlist32.n_insns, sizeof(*insns), GFP_KERNEL);
 	if (!insns)
 		return -ENOMEM;
-- 
2.47.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 5.10.y] comedi: comedi_test: Fix possible deletion of uninitialized timers
@ 2025-07-24 18:16 Ian Abbott
  2025-07-25 23:24 ` Sasha Levin
  0 siblings, 1 reply; 21+ messages in thread
From: Ian Abbott @ 2025-07-24 18:16 UTC (permalink / raw)
  To: stable; +Cc: Ian Abbott, Greg Kroah-Hartman

[ Upstream commit 1b98304c09a0192598d0767f1eb8c83d7e793091 ]

In `waveform_common_attach()`, the two timers `&devpriv->ai_timer` and
`&devpriv->ao_timer` are initialized after the allocation of the device
private data by `comedi_alloc_devpriv()` and the subdevices by
`comedi_alloc_subdevices()`.  The function may return with an error
between those function calls.  In that case, `waveform_detach()` will be
called by the Comedi core to clean up.  The check that
`waveform_detach()` uses to decide whether to delete the timers is
incorrect.  It only checks that the device private data was allocated,
but that does not guarantee that the timers were initialized.  It also
needs to check that the subdevices were allocated.  Fix it.

Fixes: 73e0e4dfed4c ("staging: comedi: comedi_test: fix timer lock-up")
Cc: stable@vger.kernel.org # 6.15+
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Link: https://lore.kernel.org/r/20250708130627.21743-1-abbotti@mev.co.uk
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
[ changed timer_delete_sync() to del_timer_sync() ]
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/comedi_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c
index bea9a3adf08c..f5199474c0e9 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -790,7 +790,7 @@ static void waveform_detach(struct comedi_device *dev)
 {
 	struct waveform_private *devpriv = dev->private;
 
-	if (devpriv) {
+	if (devpriv && dev->n_subdevices) {
 		del_timer_sync(&devpriv->ai_timer);
 		del_timer_sync(&devpriv->ao_timer);
 	}
-- 
2.47.2


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* FAILED: patch "[PATCH] comedi: comedi_test: Fix possible deletion of uninitialized" failed to apply to 5.10-stable tree
@ 2025-07-21 11:27 gregkh
  2025-07-22 21:46 ` [PATCH 5.10.y] comedi: comedi_test: Fix possible deletion of uninitialized timers Sasha Levin
  0 siblings, 1 reply; 21+ messages in thread
From: gregkh @ 2025-07-21 11:27 UTC (permalink / raw)
  To: abbotti, gregkh; +Cc: stable


The patch below does not apply to the 5.10-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-5.10.y
git checkout FETCH_HEAD
git cherry-pick -x 1b98304c09a0192598d0767f1eb8c83d7e793091
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<stable@vger.kernel.org>' --in-reply-to '2025072155-viper-viewer-98bd@gregkh' --subject-prefix 'PATCH 5.10.y' HEAD^..

Possible dependencies:



thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 1b98304c09a0192598d0767f1eb8c83d7e793091 Mon Sep 17 00:00:00 2001
From: Ian Abbott <abbotti@mev.co.uk>
Date: Tue, 8 Jul 2025 14:06:27 +0100
Subject: [PATCH] comedi: comedi_test: Fix possible deletion of uninitialized
 timers

In `waveform_common_attach()`, the two timers `&devpriv->ai_timer` and
`&devpriv->ao_timer` are initialized after the allocation of the device
private data by `comedi_alloc_devpriv()` and the subdevices by
`comedi_alloc_subdevices()`.  The function may return with an error
between those function calls.  In that case, `waveform_detach()` will be
called by the Comedi core to clean up.  The check that
`waveform_detach()` uses to decide whether to delete the timers is
incorrect.  It only checks that the device private data was allocated,
but that does not guarantee that the timers were initialized.  It also
needs to check that the subdevices were allocated.  Fix it.

Fixes: 73e0e4dfed4c ("staging: comedi: comedi_test: fix timer lock-up")
Cc: stable@vger.kernel.org # 6.15+
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Link: https://lore.kernel.org/r/20250708130627.21743-1-abbotti@mev.co.uk
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/comedi/drivers/comedi_test.c b/drivers/comedi/drivers/comedi_test.c
index 9747e6d1f6eb..7984950f0f99 100644
--- a/drivers/comedi/drivers/comedi_test.c
+++ b/drivers/comedi/drivers/comedi_test.c
@@ -792,7 +792,7 @@ static void waveform_detach(struct comedi_device *dev)
 {
 	struct waveform_private *devpriv = dev->private;
 
-	if (devpriv) {
+	if (devpriv && dev->n_subdevices) {
 		timer_delete_sync(&devpriv->ai_timer);
 		timer_delete_sync(&devpriv->ao_timer);
 	}


^ permalink raw reply related	[flat|nested] 21+ messages in thread
end of thread, other threads:[~2025-07-26  1:01 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-24 18:12 [PATCH 5.10.y] comedi: Fail COMEDI_INSNLIST ioctl if n_insns is too large Ian Abbott
2025-07-24 18:12 ` [PATCH 5.10.y] comedi: Fix some signed shift left operations Ian Abbott
2025-07-25 23:24   ` Sasha Levin
2025-07-24 18:12 ` [PATCH 5.10.y] comedi: das16m1: Fix bit shift out of bounds Ian Abbott
2025-07-25 23:24   ` Sasha Levin
2025-07-24 18:12 ` [PATCH 5.10.y] comedi: pcl812: " Ian Abbott
2025-07-25 23:24   ` Sasha Levin
2025-07-24 18:12 ` [PATCH 5.10.y] comedi: aio_iiro_16: " Ian Abbott
2025-07-25 23:26   ` Sasha Levin
2025-07-24 18:12 ` [PATCH 5.10.y] comedi: das6402: " Ian Abbott
2025-07-25 23:24   ` Sasha Levin
2025-07-24 18:12 ` [PATCH 5.10.y] comedi: Fix use of uninitialized data in insn_rw_emulate_bits() Ian Abbott
2025-07-25 23:24   ` Sasha Levin
2025-07-24 18:12 ` [PATCH 5.10.y] comedi: Fix initialization of data for instructions that write to subdevice Ian Abbott
2025-07-26  0:24   ` Sasha Levin
2025-07-24 18:12 ` [PATCH 5.10.y] comedi: comedi_test: Fix possible deletion of uninitialized timers Ian Abbott
2025-07-26  1:01   ` Sasha Levin
2025-07-26  1:01 ` [PATCH 5.10.y] comedi: Fail COMEDI_INSNLIST ioctl if n_insns is too large Sasha Levin
  -- strict thread matches above, loose matches on Subject: below --
2025-07-24 18:16 [PATCH 5.10.y] comedi: comedi_test: Fix possible deletion of uninitialized timers Ian Abbott
2025-07-25 23:24 ` Sasha Levin
2025-07-21 11:27 FAILED: patch "[PATCH] comedi: comedi_test: Fix possible deletion of uninitialized" failed to apply to 5.10-stable tree gregkh
2025-07-22 21:46 ` [PATCH 5.10.y] comedi: comedi_test: Fix possible deletion of uninitialized timers Sasha Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox