[PATCH 5.4.y] comedi: Fix initialization of data for instructions that write to subdevice
[ Upstream commit 46d8c744136ce2454aa4c35c138cc06817f92b8e ]
Some Comedi subdevice instruction handlers are known to access instruction data elements beyond the first `insn->n` elements in some cases. The `do_insn_ioctl()` and `do_insnlist_ioctl()` functions allocate at least `MIN_SAMPLES` (16) data elements to deal with this, but they do not initialize all of that. For Comedi instruction codes that write to the subdevice, the first `insn->n` data elements are copied from user-space, but the remaining elements are left uninitialized. That could be a problem if the subdevice instruction handler reads the uninitialized data. Ensure that the first `MIN_SAMPLES` elements are initialized before calling these instruction handlers, filling the uncopied elements with 0. For `do_insnlist_ioctl()`, the same data buffer elements are used for handling a list of instructions, so ensure the first `MIN_SAMPLES` elements are initialized for each instruction that writes to the subdevice.
Fixes: ed9eccbe8970 ("Staging: add comedi core") Cc: stable@vger.kernel.org # 5.13+ Signed-off-by: Ian Abbott abbotti@mev.co.uk Link: https://lore.kernel.org/r/20250707161439.88385-1-abbotti@mev.co.uk Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org [ Reworked for before commit bac42fb21259 ("comedi: get rid of compat_alloc_user_space() mess in COMEDI_CMD{,TEST} compat") ] Signed-off-by: Ian Abbott abbotti@mev.co.uk --- drivers/staging/comedi/comedi_fops.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index 413863bc929b..e4dfb5e7843d 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -1544,7 +1544,7 @@ static int do_insnlist_ioctl(struct comedi_device *dev, if (copy_from_user(&insnlist, arg, sizeof(insnlist))) return -EFAULT;
- ret = check_insnlist_len(dev, insnlist32.n_insns); + ret = check_insnlist_len(dev, insnlist.n_insns); if (ret) return ret; insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL); @@ -1580,21 +1580,27 @@ static int do_insnlist_ioctl(struct comedi_device *dev, }
for (i = 0; i < insnlist.n_insns; ++i) { + unsigned int n = insns[i].n; + if (insns[i].insn & INSN_MASK_WRITE) { if (copy_from_user(data, insns[i].data, - insns[i].n * sizeof(unsigned int))) { + n * sizeof(unsigned int))) { dev_dbg(dev->class_dev, "copy_from_user failed\n"); ret = -EFAULT; goto error; } + if (n < MIN_SAMPLES) { + memset(&data[n], 0, (MIN_SAMPLES - n) * + sizeof(unsigned int)); + } } ret = parse_insn(dev, insns + i, data, file); if (ret < 0) goto error; if (insns[i].insn & INSN_MASK_READ) { if (copy_to_user(insns[i].data, data, - insns[i].n * sizeof(unsigned int))) { + n * sizeof(unsigned int))) { dev_dbg(dev->class_dev, "copy_to_user failed\n"); ret = -EFAULT; @@ -1661,6 +1667,10 @@ static int do_insn_ioctl(struct comedi_device *dev, ret = -EFAULT; goto error; } + if (insn.n < MIN_SAMPLES) { + memset(&data[insn.n], 0, + (MIN_SAMPLES - insn.n) * sizeof(unsigned int)); + } } ret = parse_insn(dev, &insn, data, file); if (ret < 0)
[ Sasha's backport helper bot ]
Hi,
Summary of potential issues: ❌ Patch application failures detected
The upstream commit SHA1 provided is correct: 46d8c744136ce2454aa4c35c138cc06817f92b8e
Status in newer kernel trees: 6.15.y | Not found 6.12.y | Not found 6.6.y | Not found 6.1.y | Not found 5.15.y | Not found 5.10.y | Not found
Note: Could not generate a diff with upstream commit: --- Note: Could not generate diff - patch failed to apply for comparison ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | origin/linux-5.4.y | Failed | N/A |
On 26/07/2025 00:24, Sasha Levin wrote:
[ Sasha's backport helper bot ]
Hi,
Summary of potential issues: ❌ Patch application failures detected
The upstream commit SHA1 provided is correct: 46d8c744136ce2454aa4c35c138cc06817f92b8e
Status in newer kernel trees: 6.15.y | Not found 6.12.y | Not found 6.6.y | Not found 6.1.y | Not found 5.15.y | Not found 5.10.y | Not found
Note: Could not generate a diff with upstream commit:
Note: Could not generate diff - patch failed to apply for comparison
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | origin/linux-5.4.y | Failed | N/A |
Sorry about that. I was building a list of patches at the same time and applied a fixup to the wrong one.
[ Upstream commit 46d8c744136ce2454aa4c35c138cc06817f92b8e ]
Some Comedi subdevice instruction handlers are known to access instruction data elements beyond the first `insn->n` elements in some cases. The `do_insn_ioctl()` and `do_insnlist_ioctl()` functions allocate at least `MIN_SAMPLES` (16) data elements to deal with this, but they do not initialize all of that. For Comedi instruction codes that write to the subdevice, the first `insn->n` data elements are copied from user-space, but the remaining elements are left uninitialized. That could be a problem if the subdevice instruction handler reads the uninitialized data. Ensure that the first `MIN_SAMPLES` elements are initialized before calling these instruction handlers, filling the uncopied elements with 0. For `do_insnlist_ioctl()`, the same data buffer elements are used for handling a list of instructions, so ensure the first `MIN_SAMPLES` elements are initialized for each instruction that writes to the subdevice.
Fixes: ed9eccbe8970 ("Staging: add comedi core") Cc: stable@vger.kernel.org # 5.13+ Signed-off-by: Ian Abbott abbotti@mev.co.uk Link: https://lore.kernel.org/r/20250707161439.88385-1-abbotti@mev.co.uk Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org [ Reworked for before commit bac42fb21259 ("comedi: get rid of compat_alloc_user_space() mess in COMEDI_CMD{,TEST} compat") ] Signed-off-by: Ian Abbott abbotti@mev.co.uk --- v2: Fixed a patch application error due to fixing up the wrong commit. --- drivers/staging/comedi/comedi_fops.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index 45e868badee4..e4dfb5e7843d 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -1580,21 +1580,27 @@ static int do_insnlist_ioctl(struct comedi_device *dev, }
for (i = 0; i < insnlist.n_insns; ++i) { + unsigned int n = insns[i].n; + if (insns[i].insn & INSN_MASK_WRITE) { if (copy_from_user(data, insns[i].data, - insns[i].n * sizeof(unsigned int))) { + n * sizeof(unsigned int))) { dev_dbg(dev->class_dev, "copy_from_user failed\n"); ret = -EFAULT; goto error; } + if (n < MIN_SAMPLES) { + memset(&data[n], 0, (MIN_SAMPLES - n) * + sizeof(unsigned int)); + } } ret = parse_insn(dev, insns + i, data, file); if (ret < 0) goto error; if (insns[i].insn & INSN_MASK_READ) { if (copy_to_user(insns[i].data, data, - insns[i].n * sizeof(unsigned int))) { + n * sizeof(unsigned int))) { dev_dbg(dev->class_dev, "copy_to_user failed\n"); ret = -EFAULT; @@ -1661,6 +1667,10 @@ static int do_insn_ioctl(struct comedi_device *dev, ret = -EFAULT; goto error; } + if (insn.n < MIN_SAMPLES) { + memset(&data[insn.n], 0, + (MIN_SAMPLES - insn.n) * sizeof(unsigned int)); + } } ret = parse_insn(dev, &insn, data, file); if (ret < 0)
[ Sasha's backport helper bot ]
Hi,
✅ All tests passed successfully. No issues detected. No action required from the submitter.
The upstream commit SHA1 provided is correct: 46d8c744136ce2454aa4c35c138cc06817f92b8e
Status in newer kernel trees: 6.15.y | Not found 6.12.y | Not found 6.6.y | Not found 6.1.y | Not found 5.15.y | Not found 5.10.y | Not found
Note: The patch differs from the upstream commit: --- 1: 46d8c744136c ! 1: 8293aa18dbe9 comedi: Fix initialization of data for instructions that write to subdevice @@ Metadata ## Commit message ## comedi: Fix initialization of data for instructions that write to subdevice
+ [ Upstream commit 46d8c744136ce2454aa4c35c138cc06817f92b8e ] + Some Comedi subdevice instruction handlers are known to access instruction data elements beyond the first `insn->n` elements in some cases. The `do_insn_ioctl()` and `do_insnlist_ioctl()` functions @@ Commit message Signed-off-by: Ian Abbott abbotti@mev.co.uk Link: https://lore.kernel.org/r/20250707161439.88385-1-abbotti@mev.co.uk Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org + [ Reworked for before commit bac42fb21259 ("comedi: get rid of compat_alloc_user_space() mess in COMEDI_CMD{,TEST} compat") ] + Signed-off-by: Ian Abbott abbotti@mev.co.uk
- ## drivers/comedi/comedi_fops.c ## -@@ drivers/comedi/comedi_fops.c: static int do_insnlist_ioctl(struct comedi_device *dev, + ## drivers/staging/comedi/comedi_fops.c ## +@@ drivers/staging/comedi/comedi_fops.c: static int do_insnlist_ioctl(struct comedi_device *dev, }
- for (i = 0; i < n_insns; ++i) { + for (i = 0; i < insnlist.n_insns; ++i) { + unsigned int n = insns[i].n; + if (insns[i].insn & INSN_MASK_WRITE) { @@ drivers/comedi/comedi_fops.c: static int do_insnlist_ioctl(struct comedi_device dev_dbg(dev->class_dev, "copy_to_user failed\n"); ret = -EFAULT; -@@ drivers/comedi/comedi_fops.c: static int do_insn_ioctl(struct comedi_device *dev, +@@ drivers/staging/comedi/comedi_fops.c: static int do_insn_ioctl(struct comedi_device *dev, ret = -EFAULT; goto error; } -+ if (insn->n < MIN_SAMPLES) { -+ memset(&data[insn->n], 0, -+ (MIN_SAMPLES - insn->n) * sizeof(unsigned int)); ++ if (insn.n < MIN_SAMPLES) { ++ memset(&data[insn.n], 0, ++ (MIN_SAMPLES - insn.n) * sizeof(unsigned int)); + } } - ret = parse_insn(dev, insn, data, file); + ret = parse_insn(dev, &insn, data, file); if (ret < 0)
---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | origin/linux-5.4.y | Success | Success |
linux-stable-mirror@lists.linaro.org
-
Ian Abbott -
Sasha Levin