* [patch] Input: force feedback - potential integer wrap in input_ff_create()
@ 2011-10-09 16:25 Dan Carpenter
2011-10-10 5:08 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2011-10-09 16:25 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, kernel-janitors
Smatch complains about max_effects because it's an int and we cap the
maximum size, but we don't check for negative. A negative value here
could make "ff" smaller than sizeof(struct ff_device) and lead to
memory corruption.
I think max_effects can come from ->ff_effects_max in
uinput_setup_device() and that comes from the user so potentially
it could be negative. The call path is that uinput_setup_device()
sets the value in the ->private_data struct. From there it is:
-> uinput_ioctl_handler()
-> uinput_create_device()
-> input_ff_create(dev, udev->ff_effects_max);
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index 3367f76..12422ed 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -319,6 +319,12 @@ int input_ff_create(struct input_dev *dev, int max_effects)
return -EINVAL;
}
+ if (max_effects < 0)
+ return -EINVAL;
+ if (sizeof(struct ff_device) + max_effects * sizeof(struct file *) <
+ max_effects)
+ return -EINVAL;
+
ff = kzalloc(sizeof(struct ff_device) +
max_effects * sizeof(struct file *), GFP_KERNEL);
if (!ff)
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [patch] Input: force feedback - potential integer wrap in input_ff_create() 2011-10-09 16:25 [patch] Input: force feedback - potential integer wrap in input_ff_create() Dan Carpenter @ 2011-10-10 5:08 ` Dmitry Torokhov 2011-10-10 20:48 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Dmitry Torokhov @ 2011-10-10 5:08 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-input, kernel-janitors Hi Dan, On Sun, Oct 09, 2011 at 07:25:24PM +0300, Dan Carpenter wrote: > Smatch complains about max_effects because it's an int and we cap the > maximum size, but we don't check for negative. A negative value here > could make "ff" smaller than sizeof(struct ff_device) and lead to > memory corruption. > > I think max_effects can come from ->ff_effects_max in > uinput_setup_device() and that comes from the user so potentially > it could be negative. The call path is that uinput_setup_device() > sets the value in the ->private_data struct. From there it is: > -> uinput_ioctl_handler() > -> uinput_create_device() > -> input_ff_create(dev, udev->ff_effects_max); > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c > index 3367f76..12422ed 100644 > --- a/drivers/input/ff-core.c > +++ b/drivers/input/ff-core.c > @@ -319,6 +319,12 @@ int input_ff_create(struct input_dev *dev, int max_effects) > return -EINVAL; > } > > + if (max_effects < 0) > + return -EINVAL; > + if (sizeof(struct ff_device) + max_effects * sizeof(struct file *) < > + max_effects) > + return -EINVAL; > + Instead of doing this why don't we mark all relevant fields as unsigned int? Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Input: force feedback - potential integer wrap in input_ff_create() 2011-10-10 5:08 ` Dmitry Torokhov @ 2011-10-10 20:48 ` Dan Carpenter 2011-10-11 21:19 ` [patch v2] " Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2011-10-10 20:48 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, kernel-janitors On Sun, Oct 09, 2011 at 10:08:52PM -0700, Dmitry Torokhov wrote: > Instead of doing this why don't we mark all relevant fields as unsigned > int? > Sure. We'd still have to check for wrapping on 32bit I think... I'll sent a patch for this tomorrow. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* [patch v2] Input: force feedback - potential integer wrap in input_ff_create() 2011-10-10 20:48 ` Dan Carpenter @ 2011-10-11 21:19 ` Dan Carpenter 2011-10-13 4:36 ` Dmitry Torokhov 2011-10-15 14:24 ` walter harms 0 siblings, 2 replies; 6+ messages in thread From: Dan Carpenter @ 2011-10-11 21:19 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, kernel-janitors The problem here is that max_effects can wrap on 32 bits systems. We'd allocate a smaller amount of data than sizeof(struct ff_device). The call to kcalloc() on the next line would fail but it would write the NULL return outside of the memory we just allocated causing data corruption. The call path is that uinput_setup_device() get ->ff_effects_max from the user and sets the value in the ->private_data struct. From there it is: -> uinput_ioctl_handler() -> uinput_create_device() -> input_ff_create(dev, udev->ff_effects_max); I've also changed ff_effects_max so it's an unsigned int instead of a signed int as a cleanup. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- V2: made max_effects unsigned diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c index 3367f76..3051c84 100644 --- a/drivers/input/ff-core.c +++ b/drivers/input/ff-core.c @@ -309,7 +309,7 @@ EXPORT_SYMBOL_GPL(input_ff_event); * Once ff device is created you need to setup its upload, erase, * playback and other handlers before registering input device */ -int input_ff_create(struct input_dev *dev, int max_effects) +int input_ff_create(struct input_dev *dev, unsigned int max_effects) { struct ff_device *ff; int i; @@ -319,6 +319,10 @@ int input_ff_create(struct input_dev *dev, int max_effects) return -EINVAL; } + if (sizeof(struct ff_device) + max_effects * sizeof(struct file *) < + max_effects) + return -EINVAL; + ff = kzalloc(sizeof(struct ff_device) + max_effects * sizeof(struct file *), GFP_KERNEL); if (!ff) diff --git a/include/linux/input.h b/include/linux/input.h index 57add32..6d5eddb 100644 --- a/include/linux/input.h +++ b/include/linux/input.h @@ -1610,7 +1610,7 @@ struct ff_device { struct file *effect_owners[]; }; -int input_ff_create(struct input_dev *dev, int max_effects); +int input_ff_create(struct input_dev *dev, unsigned int max_effects); void input_ff_destroy(struct input_dev *dev); int input_ff_event(struct input_dev *dev, unsigned int type, unsigned int code, int value); diff --git a/include/linux/uinput.h b/include/linux/uinput.h index d28c726..2aa2881 100644 --- a/include/linux/uinput.h +++ b/include/linux/uinput.h @@ -68,7 +68,7 @@ struct uinput_device { unsigned char head; unsigned char tail; struct input_event buff[UINPUT_BUFFER_SIZE]; - int ff_effects_max; + unsigned int ff_effects_max; struct uinput_request *requests[UINPUT_NUM_REQUESTS]; wait_queue_head_t requests_waitq; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch v2] Input: force feedback - potential integer wrap in input_ff_create() 2011-10-11 21:19 ` [patch v2] " Dan Carpenter @ 2011-10-13 4:36 ` Dmitry Torokhov 2011-10-15 14:24 ` walter harms 1 sibling, 0 replies; 6+ messages in thread From: Dmitry Torokhov @ 2011-10-13 4:36 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-input, kernel-janitors On Wed, Oct 12, 2011 at 12:19:49AM +0300, Dan Carpenter wrote: > The problem here is that max_effects can wrap on 32 bits systems. > We'd allocate a smaller amount of data than sizeof(struct ff_device). > The call to kcalloc() on the next line would fail but it would write > the NULL return outside of the memory we just allocated causing data > corruption. > > The call path is that uinput_setup_device() get ->ff_effects_max from > the user and sets the value in the ->private_data struct. From there > it is: > -> uinput_ioctl_handler() > -> uinput_create_device() > -> input_ff_create(dev, udev->ff_effects_max); > > I've also changed ff_effects_max so it's an unsigned int instead of > a signed int as a cleanup. Applied (adding a temp to hold the size), thanks. -- Dmitry ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch v2] Input: force feedback - potential integer wrap in input_ff_create() 2011-10-11 21:19 ` [patch v2] " Dan Carpenter 2011-10-13 4:36 ` Dmitry Torokhov @ 2011-10-15 14:24 ` walter harms 1 sibling, 0 replies; 6+ messages in thread From: walter harms @ 2011-10-15 14:24 UTC (permalink / raw) To: Dan Carpenter; +Cc: Dmitry Torokhov, linux-input, kernel-janitors Am 11.10.2011 23:19, schrieb Dan Carpenter: > The problem here is that max_effects can wrap on 32 bits systems. > We'd allocate a smaller amount of data than sizeof(struct ff_device). > The call to kcalloc() on the next line would fail but it would write > the NULL return outside of the memory we just allocated causing data > corruption. > > The call path is that uinput_setup_device() get ->ff_effects_max from > the user and sets the value in the ->private_data struct. From there > it is: > -> uinput_ioctl_handler() > -> uinput_create_device() > -> input_ff_create(dev, udev->ff_effects_max); > > I've also changed ff_effects_max so it's an unsigned int instead of > a signed int as a cleanup. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > V2: made max_effects unsigned > > diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c > index 3367f76..3051c84 100644 > --- a/drivers/input/ff-core.c > +++ b/drivers/input/ff-core.c > @@ -309,7 +309,7 @@ EXPORT_SYMBOL_GPL(input_ff_event); > * Once ff device is created you need to setup its upload, erase, > * playback and other handlers before registering input device > */ > -int input_ff_create(struct input_dev *dev, int max_effects) > +int input_ff_create(struct input_dev *dev, unsigned int max_effects) > { > struct ff_device *ff; > int i; > @@ -319,6 +319,10 @@ int input_ff_create(struct input_dev *dev, int max_effects) > return -EINVAL; > } > > + if (sizeof(struct ff_device) + max_effects * sizeof(struct file *) < > + max_effects) > + return -EINVAL; > + i am not sure if that is the way to go. the minimum size you need is sizeof(struct ff_device)+sizeof(struct file *) (assuming that max_effects>=1). If the input can be outside any useful boundaries i would go for this: uint64_t tmp= sizeof(struct ff_device) + max_effects * sizeof(struct file *) ; if (tmp >= UINT_MAX ) ...... Clearly it is better to have max_effects in a proper range. re, wh > ff = kzalloc(sizeof(struct ff_device) + > max_effects * sizeof(struct file *), GFP_KERNEL); > if (!ff) > diff --git a/include/linux/input.h b/include/linux/input.h > index 57add32..6d5eddb 100644 > --- a/include/linux/input.h > +++ b/include/linux/input.h > @@ -1610,7 +1610,7 @@ struct ff_device { > struct file *effect_owners[]; > }; > > -int input_ff_create(struct input_dev *dev, int max_effects); > +int input_ff_create(struct input_dev *dev, unsigned int max_effects); > void input_ff_destroy(struct input_dev *dev); > > int input_ff_event(struct input_dev *dev, unsigned int type, unsigned int code, int value); > diff --git a/include/linux/uinput.h b/include/linux/uinput.h > index d28c726..2aa2881 100644 > --- a/include/linux/uinput.h > +++ b/include/linux/uinput.h > @@ -68,7 +68,7 @@ struct uinput_device { > unsigned char head; > unsigned char tail; > struct input_event buff[UINPUT_BUFFER_SIZE]; > - int ff_effects_max; > + unsigned int ff_effects_max; > > struct uinput_request *requests[UINPUT_NUM_REQUESTS]; > wait_queue_head_t requests_waitq; > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-10-15 14:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-09 16:25 [patch] Input: force feedback - potential integer wrap in input_ff_create() Dan Carpenter 2011-10-10 5:08 ` Dmitry Torokhov 2011-10-10 20:48 ` Dan Carpenter 2011-10-11 21:19 ` [patch v2] " Dan Carpenter 2011-10-13 4:36 ` Dmitry Torokhov 2011-10-15 14:24 ` walter harms
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).