On 25/10/18 02:36, Spencer E. Olson wrote:
Changes do_insn*_ioctl functions to allow for data lengths for each
comedi_insn of up to 2^16. This patch also changes these functions to only
allocate as much memory as is necessary for each comedi_insn, rather than
allocating a fixed-sized scratch space.
In testing some user-space code for the new INSN_DEVICE_CONFIG_GET_ROUTES
facility with some newer hardware, I discovered that do_insn_ioctl and
do_insnlist_ioctl limited the amount of data that can be passed into the
kernel for insn's to a length of 256. For some newer hardware, the number
of routes can be greater than 1000. Working around the old limits (256)
would complicate the user-space/kernel interaction.
The new upper limit is reasonable with current memory available and does
not otherwise impact the memory footprint for any current or otherwise
typical configuration.
Signed-off-by: Spencer E. Olson <olsonse@xxxxxxxxx>
---
drivers/staging/comedi/comedi_fops.c | 37 +++++++++++++++++-----------
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index c1c6b2b4ab91..a163ec2df872 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1500,7 +1500,7 @@ static int parse_insn(struct comedi_device *dev, struct comedi_insn *insn,
* data (for reads) to insns[].data pointers
*/
/* arbitrary limits */
-#define MAX_SAMPLES 256
+#define MAX_SAMPLES 65536
static int do_insnlist_ioctl(struct comedi_device *dev,
struct comedi_insnlist __user *arg, void *file)
{
@@ -1513,12 +1513,6 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
if (copy_from_user(&insnlist, arg, sizeof(insnlist)))
return -EFAULT;
- data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
- if (!data) {
- ret = -ENOMEM;
- goto error;
- }
-
insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL);
if (!insns) {
ret = -ENOMEM;
@@ -1539,6 +1533,14 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
ret = -EINVAL;
goto error;
}
+
+ data = kmalloc_array(insns[i].n, sizeof(unsigned int),
+ GFP_KERNEL);
+ if (!data) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
if (insns[i].insn & INSN_MASK_WRITE) {
if (copy_from_user(data, insns[i].data,
insns[i].n * sizeof(unsigned int))) {
@@ -1560,6 +1562,11 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
goto error;
}
}
+
+ /* free up the single-instruction data memory */
+ kfree(data);
+ data = NULL;
+
if (need_resched())
schedule();
}
@@ -1594,20 +1601,20 @@ static int do_insn_ioctl(struct comedi_device *dev,
unsigned int *data = NULL;
int ret = 0;
- data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
- if (!data) {
- ret = -ENOMEM;
- goto error;
- }
-
if (copy_from_user(&insn, arg, sizeof(insn))) {
- ret = -EFAULT;
- goto error;
+ return -EFAULT;
}
/* This is where the behavior of insn and insnlist deviate. */
if (insn.n > MAX_SAMPLES)
insn.n = MAX_SAMPLES;
+
+ data = kmalloc_array(insn.n, sizeof(unsigned int), GFP_KERNEL);
+ if (!data) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
if (insn.insn & INSN_MASK_WRITE) {
if (copy_from_user(data,
insn.data,
There are still some broken drivers that do not check insn->n and assume
the data has some minimum length, so we will need to make the data array
some minimum length (say 16 * sizeof(unsigned int)) until those drivers
have been fixed.
For example (there might be other places) ...
In cb_pcidas64.c:
* ao_winsn() (the insn_write handler for the AO subdevice) assumes that
insn->n is 1 and reads data[0], but insn->n could be 0.
* eeprom_read_insn() (the insn_read handler for the serial EEPROM) also
assumes insn->n is 1 and accesses data[0].
* ai_config_master_clock() (the handler for INSN_CONFIG_TIMER_1) assumes
insn->n is at least 5 but nothing checks it. (It isn't checked by
check_insn_config_length() in comedi_fops.c currently.)
In cb_pcidda.c:
* cb_pcidda_ao_insn_write() assumes insn->n is 1 and that data[0] is valid.
In ni_labpc_common.c:
* labpc_ao_insn_write() assumes insn->n is 1 and that data[0] is valid.
In ni_mio_common.c:
* ni_calib_insn_read(), ni_eeprom_insn_read(),
ni_m_series_eeprom_insn_read(), ni_calib_insn_write() all assume insn->n
is 1 and that data[0] is valid or accessible.
In s526.c:
* s526_gpct_winsn() uses data[0] and sometimes data[1] but doesn't check
insn->n.
* s526_gpct_insn_config() accesses up to data[3] but doesn't check the
length (and check_insn_config_length() in comedi_fops.c doesn't check
the lengths for the INSN_CONFIG instructions handled here).
--
-=( Ian Abbott <abbotti@xxxxxxxxx> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address: )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
