Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220531074117.610321-1-gopalkrishna.tiwari@gmail.com> <20220531074117.610321-11-gopalkrishna.tiwari@gmail.com>
In-Reply-To: <20220531074117.610321-11-gopalkrishna.tiwari@gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 31 May 2022 13:22:55 -0700
Message-ID: <CABBYNZJqnj0x-7wheW8WG9p6oP2DqLh9PBuoqT6K0QmhW5g5uw@mail.gmail.com>
Subject: Re: [Bluez V2 10/13] Fixing use after free in src/device.c
To:     Gopal Tiwari <gopalkrishna.tiwari@gmail.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Gopal Tiwari <gtiwari@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Gopal,

On Tue, May 31, 2022 at 12:42 AM Gopal Tiwari
<gopalkrishna.tiwari@gmail.com> wrote:
>
> From: Gopal Tiwari <gtiwari@redhat.com>
>
> Following traces reported by covirty tool
>
> Error: USE_AFTER_FREE (CWE-416):
> bluez-5.64/src/device.c:2962: path: Condition
> "!dbus_message_get_args(msg, NULL, 0 /* (int)0 */)", taking false branch.
>
> bluez-5.64/src/device.c:2965: path:
> Condition "device->bonding", taking false branch.
>
> bluez-5.64/src/device.c:2968: path:
> Condition "device->bredr_state.bonded", taking true branch.
>
> bluez-5.64/src/device.c:2969: path: Falling through to end of
> if statement.
>
> bluez-5.64/src/device.c:2977: path: Condition "state->bonded",
> taking false branch.
>
> bluez-5.64/src/device.c:2983: path: Condition "agent", taking
> true branch.
>
> bluez-5.64/src/device.c:2984: path: Falling through to end of
> if statement.
>
> bluez-5.64/src/device.c:2990: path: Condition "agent", taking
> true branch.
>
> bluez-5.64/src/device.c:3005: path: Condition "bdaddr_type != 0",
> taking true branch.
>
> bluez-5.64/src/device.c:3006: path:
>
> Condition "!state->connected", taking true branch.
>
> bluez-5.64/src/device.c:3006: path:
> Condition "btd_le_connect_before_pairing()", taking true branch.
> bluez-5.64/src/device.c:3007: freed_arg: "device_connect_le" frees
> "device->bonding".
>
> bluez-5.64/src/device.c:3007: path: Falling through to end of
> if statement.
>
> bluez-5.64/src/device.c:3012: path: Falling through to end of
> if statement.
>
> bluez-5.64/src/device.c:3017: path: Condition "err < 0",
> taking true branch.
>
> bluez-5.64/src/device.c:3018: double_free: Calling "bonding_request_free"
> frees pointer "device->bonding" which has already been freed.
>
> Signed-off-by: Gopal Tiwari <gtiwari@redhat.com>
> ---
>  src/device.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/device.c b/src/device.c
> index 8dc12d026..a0e5d40db 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -2942,6 +2942,7 @@ static void bonding_request_free(struct bonding_req *bonding)
>                 bonding->device->bonding = NULL;
>
>         g_free(bonding);
> +       bonding = NULL;

I don't think this fixes anything really, since bonding variable goes
out of scope this won't change anything, in fact this seems to be a
false positive since device->bonding shall be set to NULL in the if
statement just above any other call to bonding_request_free will bail
out when checking !bonding, it would be a problem if and only if the
code was using bonding pointer directly instead of device->bonding
with bonding_request_free.

>  }
>
>  static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg,
> --
> 2.26.2
>


-- 
Luiz Augusto von Dentz