Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.

[Gopal Tiwari <gtiwari@redhat.com>]

Hi Luiz, 

We are using a tool to review code and generate warning as an when it sees any of the problem scenarios like use_after_free etc.. 
 
----- Original Message -----
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
To: Gopal Tiwari <gtiwari@redhat.com>
Cc: linux-bluetooth@vger.kernel.org, Don Zickus <dzickus@redhat.com>
Sent: Fri, 20 Jul 2018 04:24:34 -0400 (EDT)
Subject: Re: [PATCH BlueZ 1/2] Fixing possible use_after_free.

>Hi Gopal,

>On Fri, Jul 20, 2018 at 7:33 AM, Gopal Tiwari <gtiwari@redhat.com> wrote:
> During the code review of code. Found some of the possible
> double free scenario's. So fixing them.

>Id like that you split the changes with possible traces when that
>happen, besides some changes seems questionable (see the comments
>bellow).

> ---
>  gobex/gobex-transfer.c | 6 ++++++
>  obexd/client/session.c | 6 +++++-
>  tools/mesh/prov-db.c   | 4 +++-
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
> index bc99306..11980d0 100644
> --- a/gobex/gobex-transfer.c
> +++ b/gobex/gobex-transfer.c
> @@ -428,6 +428,12 @@ guint g_obex_put_rsp(GObex *obex, GObexPacket *req,
>         va_start(args, first_hdr_id);
>         transfer_put_req_first(transfer, req, first_hdr_id, args);
>         va_end(args);
> +
> +       /* Checking transfer in case transfer_put_req_first has
> +        * freed it to avoid double free */
> +       if(!transfer)
> +               return 0;

>Check the code style, the should be a space after like in: if (

My mistake :(. I'll re-post it. Thanks for pointing out.   

>         if (!g_slist_find(transfers, transfer))
>                 return 0;
>
> diff --git a/obexd/client/session.c b/obexd/client/session.c
> index 5bd2d26..077224b 100644
> --- a/obexd/client/session.c
> +++ b/obexd/client/session.c
> @@ -938,7 +938,11 @@ static void session_terminate_transfer(struct obc_session *session,
>         if (session->p == NULL)
>                 session_process_queue(session);
>
> -       obc_session_unref(session);
> +       /* Verifying the session variable for double free
> +        */
> +
> +       if (session)
> +               obc_session_unref(session);

This is how we have got this .. 

bluez-5.49/obexd/client/session.c:935: freed_arg: "session_process_queue" frees "session".
bluez-5.49/obexd/client/session.c:894:2: freed_arg: "obc_session_unref" frees parameter "session".
bluez-5.49/obexd/client/session.c:304:2: freed_arg: "session_free" frees parameter "session".
bluez-5.49/obexd/client/session.c:252:2: freed_arg: "g_free" frees parameter "session".
bluez-5.49/obexd/client/session.c:937: deref_arg: Calling "obc_session_unref" dereferences freed pointer "session".
bluez-5.49/obexd/client/session.c:287:2: deref_parm: Directly dereferencing parameter "session".
#  935|   		session_process_queue(session);
#  936|   
#  937|-> 	obc_session_unref(session);


>I don't think you are fixing anything here with checking the session
>pointer since any function altering it won't be able to reset the
>pointer to NULL. If is a chance of double free here then what should
>be done is have a extra reference to prevent it to be free before but
>given the function name that should never happen so I wonder where you
>got this from?

>  }
>
>  static void session_notify_complete(struct obc_session *session,
> diff --git a/tools/mesh/prov-db.c b/tools/mesh/prov-db.c
> index 05b2547..74915d2 100644
> --- a/tools/mesh/prov-db.c
> +++ b/tools/mesh/prov-db.c
> @@ -876,8 +876,10 @@ bool prov_db_local_set_iv_index(uint32_t iv_index, bool update, bool prov)
>
>         res = true;
>  done:
> +       /* Varify in_str for double free */
>
> -       g_free(in_str);
> +       if(in_str)
> +               g_free(in_str);

bluez-5.49/mesh/prov-db.c:860: freed_arg: "g_free" frees "in_str".
bluez-5.49/mesh/prov-db.c:880: double_free: Calling "g_free" frees pointer "in_str" which has already been freed.
#  878|   done:
#  879|   
#  880|-> 	g_free(in_str);

>g_free is already NULL pointer safe.

Ok, I forgot to check that :( my fault. 

>
>         if(jmain)
>                 json_object_put(jmain);
> --
> 2.7.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gopal..

-- 
Luiz Augusto von Dentz