lkml.org 
[lkml]   [2010]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[PATCH RFC] serio HIL MLC: don't deref null, don't leak and return proper error
    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.

    Ohh and please do CC me on replies.


    Signed-off-by: Jesper Juhl <jj@chaosbits.net>
    ---
    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)


    --
    Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
    Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
    Plain text mails only, please.



    \
     
     \ /
      Last update: 2010-11-07 20:51    [W:0.025 / U:32.004 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site