On 11/09/2010 07:35 AM, Dmitry Torokhov wrote:
On Sun, Nov 07, 2010 at 08:36:33PM +0100, Jesper Juhl wrote:Hi, While reviewing various users of kernel memory allocation functions I came across drivers/input/serio/hil_mlc.c::hil_mlc_register() and noticed that - it calls kzalloc() buf fails to check for a NULL return before use. - it makes several allocations and if one fails it doesn't free the previous ones. - It doesn't return -ENOMEM in the failed memory allocation case (it just crashes). This patch corrects all of the above and also reworks the only caller of this function that I could find (drivers/input/serio/hp_sdc_mlc.c::hp_sdc_mlc_out()) so that it now checks the return value of hil_mlc_register() and properly propagate it on failure and I also restructured the code to remove some labels and goto's to make it, IMHO nicer to read. I have no hardware to test, so please review carefully and let me know if I've done something completely stupid. Please don't merge this RFC patch unless at least one or more people who know this code and can actually test it have ACK'ed it.I think Helge Deller (CCed) used to have access to such hardware...
Yes, and I tested the patch successfully. Tested-by: Helge Deller <deller@xxxxxx> Acked-by: Helge Deller <deller@xxxxxx>
Signed-off-by: Jesper Juhl<jj@xxxxxxxxxxxxx> --- hil_mlc.c | 5 +++++ hp_sdc_mlc.c | 18 +++++++++--------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c index e5624d8..bfd3865 100644 --- a/drivers/input/serio/hil_mlc.c +++ b/drivers/input/serio/hil_mlc.c @@ -932,6 +932,11 @@ int hil_mlc_register(hil_mlc *mlc) hil_mlc_copy_di_scratch(mlc, i); mlc_serio = kzalloc(sizeof(*mlc_serio), GFP_KERNEL); mlc->serio[i] = mlc_serio; + if (!mlc->serio[i]) { + for (; i>= 0; i--) + kfree(mlc->serio[i]); + return -ENOMEM; + } snprintf(mlc_serio->name, sizeof(mlc_serio->name)-1, "HIL_SERIO%d", i); snprintf(mlc_serio->phys, sizeof(mlc_serio->phys)-1, "HIL%d", i); mlc_serio->id = hil_mlc_serio_id; diff --git a/drivers/input/serio/hp_sdc_mlc.c b/drivers/input/serio/hp_sdc_mlc.c index 7d2b820..d50f067 100644 --- a/drivers/input/serio/hp_sdc_mlc.c +++ b/drivers/input/serio/hp_sdc_mlc.c @@ -305,6 +305,7 @@ static void hp_sdc_mlc_out(hil_mlc *mlc) static int __init hp_sdc_mlc_init(void) { hil_mlc *mlc =&hp_sdc_mlc; + int err; #ifdef __mc68000__ if (!MACH_IS_HP300) @@ -323,22 +324,21 @@ static int __init hp_sdc_mlc_init(void) mlc->out =&hp_sdc_mlc_out; mlc->priv =&hp_sdc_mlc_priv; - if (hil_mlc_register(mlc)) { + err = hil_mlc_register(mlc); + if (err) { printk(KERN_WARNING PREFIX "Failed to register MLC structure with hil_mlc\n"); - goto err0; + return err; } if (hp_sdc_request_hil_irq(&hp_sdc_mlc_isr)) { printk(KERN_WARNING PREFIX "Request for raw HIL ISR hook denied\n"); - goto err1; + if (hil_mlc_unregister(mlc)) + printk(KERN_ERR PREFIX "Failed to unregister MLC structure with hil_mlc.\n" + "This is bad. Could cause an oops.\n"); + return -EBUSY; } + return 0; - err1: - if (hil_mlc_unregister(mlc)) - printk(KERN_ERR PREFIX "Failed to unregister MLC structure with hil_mlc.\n" - "This is bad. Could cause an oops.\n"); - err0: - return -EBUSY; } static void __exit hp_sdc_mlc_exit(void)
-- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
