lkml.org 
[lkml]   [2008]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    Subject[PATCH 2/2] irda: avoid potential memory leak in irda_setsockopt()
    From

    There are paths through the irda_setsockopt() function where we return and
    may or may not have allocated a new ias_obj but in any case have not used
    it for anything yet and we end up leaking memory.

    As far as I can tell, in the case where we didn't allocate a new ias_ob
    but simply were planning to use one already available then we should not
    free it before returning. But when we have allocated a brand new ias_obj
    but have not yet used it or put it on any lists etc and then return,
    that's a memory leak.

    There are two cases:

    1)
    switch (optname) {
    case IRLMP_IAS_SET:
    ...
    if(ias_obj == (struct ias_object *) NULL) {
    /* Create a new object */
    --[alloc]--> ias_obj = irias_new_object(ias_opt->irda_class_name,
    jiffies);
    ...
    switch(ias_opt->irda_attrib_type) {
    case IAS_OCT_SEQ:
    /* Check length */
    if(ias_opt->attribute.irda_attrib_octet_seq.len >
    IAS_MAX_OCTET_STRING) {
    kfree(ias_opt);
    --[leak]--> return -EINVAL;
    ...
    }
    irias_insert_object(ias_obj);

    The allocated object isn't referenced at all until we get outside the
    inner switch, so clearly we leak it (if we took the path that allocated it
    that is).


    2)
    The second case is the same as the above, except it's the default: case in
    the inner switch instead of case IAS_OCT_SEQ:

    default :
    kfree(ias_opt);
    return -EINVAL;


    The way I propose to fix this is with a new variable that keeps track of
    whether or not we found an existing ias_obj to use or if we took the
    allocation path, then use that variable to determine if we should free
    ias_obj before returning from the function.

    I'm not very intimate with this code, so if there's a better solution I'd
    very much like to hear it. It's also entirely possible that someone with
    more knowledge of this code can prove that these cases can't actually ever
    happen - if that's the case then please let me know.

    This patch is meant to be applied on top of
    [PATCH 1/2] irda: return -ENOMEM upon failure to allocate new ias_obj

    The Coverity checker gets credit for pointing its finger towards this.


    Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
    ---

    af_irda.c | 8 +++++++-
    1 file changed, 7 insertions(+), 1 deletion(-)

    diff --git a/net/irda/af_irda.c b/net/irda/af_irda.c
    index e33f0a5..352e8a7 100644
    --- a/net/irda/af_irda.c
    +++ b/net/irda/af_irda.c
    @@ -1824,7 +1824,8 @@ static int irda_setsockopt(struct socket *sock, int level, int optname,
    struct irda_sock *self = irda_sk(sk);
    struct irda_ias_set *ias_opt;
    struct ias_object *ias_obj;
    - struct ias_attrib * ias_attr; /* Attribute in IAS object */
    + struct ias_attrib *ias_attr; /* Attribute in IAS object */
    + int alloc_new_obj = 0;
    int opt;

    IRDA_DEBUG(2, "%s(%p)\n", __FUNCTION__, self);
    @@ -1885,6 +1886,7 @@ static int irda_setsockopt(struct socket *sock, int level, int optname,
    kfree(ias_opt);
    return -ENOMEM;
    }
    + alloc_new_obj = 1;
    }

    /* Do we have the attribute already ? */
    @@ -1908,6 +1910,8 @@ static int irda_setsockopt(struct socket *sock, int level, int optname,
    if(ias_opt->attribute.irda_attrib_octet_seq.len >
    IAS_MAX_OCTET_STRING) {
    kfree(ias_opt);
    + if (alloc_new_obj)
    + kfree(ias_obj);
    return -EINVAL;
    }
    /* Add an octet sequence attribute */
    @@ -1936,6 +1940,8 @@ static int irda_setsockopt(struct socket *sock, int level, int optname,
    break;
    default :
    kfree(ias_opt);
    + if (alloc_new_obj)
    + kfree(ias_obj);
    return -EINVAL;
    }
    irias_insert_object(ias_obj);




    \
     
     \ /
      Last update: 2008-01-13 01:43    [W:0.024 / U:31.748 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site