Re: [Bluez V2 10/13] Fixing use after free in src/device.c

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

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