Hi Bharat,
> Check if the size parameter passed to g_malloc/g_malloc0
> are non-zero. In case of invalid size allocation it returns
> NULL and aborts.
>
> Signed-off-by: Bharat Panda <bharat.panda@xxxxxxxxxxx>
> ---
> profiles/audio/a2dp.c | 7 ++++++-
> profiles/audio/avdtp.c | 10 ++++++++++
> profiles/health/hdp.c | 4 +++-
> profiles/health/mcap.c | 2 ++
> 4 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index c9dac9a..eb38e95 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -522,6 +522,8 @@ static gboolean endpoint_getcap_ind(struct avdtp *session,
> length = a2dp_sep->endpoint->get_capabilities(a2dp_sep, &capabilities,
> a2dp_sep->user_data);
>
> + if(!(sizeof(*codec_caps) + length))
> + return -EINVAL;
a) fix the coding style an b) can *codec_caps be ever 0?
> codec_caps = g_malloc0(sizeof(*codec_caps) + length);
> codec_caps->media_type = AVDTP_MEDIA_TYPE_AUDIO;
> codec_caps->media_codec_type = a2dp_sep->codec;
> @@ -1346,12 +1348,15 @@ static void select_cb(struct a2dp_setup *setup, void *ret, int size)
> goto done;
> }
>
> + if (!(sizeof(*cap) + size))
> + return -EINVAL;
Same here. Can *cap be ever an empty struct.
> + cap = g_malloc0(sizeof(*cap) + size);
> +
Don't move code around just for the sake of moving code around.
> media_transport = avdtp_service_cap_new(AVDTP_MEDIA_TRANSPORT,
> NULL, 0);
>
> setup->caps = g_slist_append(setup->caps, media_transport);
>
> - cap = g_malloc0(sizeof(*cap) + size);
> cap->media_type = AVDTP_MEDIA_TYPE_AUDIO;
> cap->media_codec_type = setup->sep->codec;
> memcpy(cap->data, ret, size);
> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index 8a7d1c0..88ea28b 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -1299,6 +1299,10 @@ static GSList *caps_to_list(uint8_t *data, int size,
> break;
> }
>
> + if (!(sizeof(struct avdtp_service_capability) + length)) {
> + error("Memory allocation failed");
> + return NULL;
> + }
I highly doubt that this will ever be 0. This is pretty much pointless and also has a complete wrong error message.
> cap = g_malloc(sizeof(struct avdtp_service_capability) +
> length);
> memcpy(cap, data, 2 + length);
> @@ -2732,6 +2736,8 @@ static int send_request(struct avdtp *session, gboolean priority,
>
> req = g_new0(struct pending_req, 1);
> req->signal_id = signal_id;
> + if (!size)
> + return -EINVAL;
Why not check this at the beginning of the function. I dislike this random insertion of checks. It makes it pretty much unreadable.
> req->data = g_malloc(size);
> memcpy(req->data, buffer, size);
> req->data_size = size;
> @@ -3285,6 +3291,8 @@ struct avdtp_service_capability *avdtp_service_cap_new(uint8_t category,
> if (category < AVDTP_MEDIA_TRANSPORT || category > AVDTP_DELAY_REPORTING)
> return NULL;
>
> + if (!(sizeof(struct avdtp_service_capability) + length))
> + return NULL;
Same here. I do not think this can ever be 0. These kind of extra code addition are fully useless. They are a result of fixing a symptom without knowing the root cause.
> cap = g_malloc(sizeof(struct avdtp_service_capability) + length);
> cap->category = category;
> cap->length = length;
> @@ -3444,6 +3452,8 @@ int avdtp_set_configuration(struct avdtp *session,
> caps_len += cap->length + 2;
> }
>
> + if (!(sizeof(struct setconf_req) + caps_len))
> + return -EINVAL;
Same here. Seriously.
> req = g_malloc0(sizeof(struct setconf_req) + caps_len);
>
> req->int_seid = lsep->info.seid;
> diff --git a/profiles/health/hdp.c b/profiles/health/hdp.c
> index 48dad52..6745230 100644
> --- a/profiles/health/hdp.c
> +++ b/profiles/health/hdp.c
> @@ -1470,11 +1470,13 @@ static void destroy_create_dc_data(gpointer data)
> hdp_create_data_unref(dc_data);
> }
>
> -static void *generate_echo_packet(void)
> +static uint8_t *generate_echo_packet(void)
> {
> uint8_t *buf;
> int i;
>
> + if(!HDP_ECHO_LEN)
> + return -EINVAL;
Seriously? HDP_ECHO_LEN is 15. It is a constant. This check is fully useless and its coding style is also wrong.
> buf = g_malloc(HDP_ECHO_LEN);
> srand(time(NULL));
>
> diff --git a/profiles/health/mcap.c b/profiles/health/mcap.c
> index 102ec85..2a0969f 100644
> --- a/profiles/health/mcap.c
> +++ b/profiles/health/mcap.c
> @@ -360,6 +360,8 @@ static int mcap_send_cmd(struct mcap_mcl *mcl, uint8_t oc, uint8_t rc,
>
> sock = g_io_channel_unix_get_fd(mcl->cc);
>
> + if (!(sizeof(mcap_rsp) + len))
> + return -EINVAL;
Same here. I about this will ever be 0.
> cmd = g_malloc(sizeof(mcap_rsp) + len);
> cmd->op = oc;
> cmd->rc = rc;
In summary, there is one candidate that might be valid. It has been asked before, please provide a backtrace for this crash and not start randomly adding if checks that are not need.
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
