lkml.org 
[lkml]   [2010]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH RFC] serio HIL MLC: don't deref null, don't leak and return proper error
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@gmx.de>
Acked-by: Helge Deller <deller@gmx.de>


>> 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)
>>
>>
>>



\
 
 \ /
  Last update: 2010-11-20 22:17    [W:0.082 / U:7.716 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site