linux-kernel - Re: [PATCH][next] Bluetooth: Use struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 28 Mar 2019 10:46:11 -0700
From:   Joe Perches <joe@...ches.com>
To:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Cc:     linux-bluetooth@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH][next] Bluetooth: Use struct_size() helper

On Thu, 2019-03-28 at 12:30 -0500, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
> 
> struct foo {
>     int stuff;
>     struct boo entry[];
> };
> 
> size = sizeof(struct foo) + count * sizeof(struct boo);
> 
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
> 
> size = struct_size(instance, entry, count);
[]
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
[]
> @@ -2301,8 +2301,7 @@ static int load_link_keys(struct sock *sk, struct hci_dev *hdev, void *data,
>  				       MGMT_STATUS_INVALID_PARAMS);
>  	}
>  
> -	expected_len = sizeof(*cp) + key_count *
> -					sizeof(struct mgmt_link_key_info);
> +	expected_len = struct_size(cp, keys, key_count);
>  	if (expected_len != len) {
>  		bt_dev_err(hdev, "load_link_keys: expected %u bytes, got %u bytes",
>  			   expected_len, len);
> @@ -5030,7 +5029,7 @@ static int load_irks(struct sock *sk, struct hci_dev *hdev, void *cp_data,
>  				       MGMT_STATUS_INVALID_PARAMS);
>  	}
>  
> -	expected_len = sizeof(*cp) + irk_count * sizeof(struct mgmt_irk_info);
> +	expected_len = struct_size(cp, irks, irk_count);
>  	if (expected_len != len) {
>  		bt_dev_err(hdev, "load_irks: expected %u bytes, got %u bytes",
>  			   expected_len, len);
> @@ -5112,8 +5111,7 @@ static int load_long_term_keys(struct sock *sk, struct hci_dev *hdev,
>  				       MGMT_STATUS_INVALID_PARAMS);
>  	}
>  
> -	expected_len = sizeof(*cp) + key_count *
> -					sizeof(struct mgmt_ltk_info); 
> +	expected_len = struct_size(cp, keys, key_count);
>  	if (expected_len != len) {
>  		bt_dev_err(hdev, "load_keys: expected %u bytes, got %u bytes",
>  			   expected_len, len);
> @@ -5847,8 +5845,7 @@ static int load_conn_param(struct sock *sk, struct hci_dev *hdev, void *data,
>  				       MGMT_STATUS_INVALID_PARAMS);
>  	}
>  
> -	expected_len = sizeof(*cp) + param_count *
> -					sizeof(struct mgmt_conn_param);
> +	expected_len = struct_size(cp, params, param_count);
>  	if (expected_len != len) {
>  		bt_dev_err(hdev, "load_conn_param: expected %u bytes, got %u bytes",
>  			   expected_len, len);

Maybe add a helper function for all of these.
Perhaps something like:

static bool verify_len(size_t actual, size_t expected)
{
	if (actual == expected)
		return true;
	bt_dev_err("%ps: expected %zu bytes, got %zu bytes",
		   __builtin_return_address(1), expected, actual);
	return false;
}

So the last block above might be

	if (!verify_len(len, struct_size(cp, params, param_count),))
		return mgmt_cmd_status(sk, hdev->id,  MGMT_OP_LOAD_CONN_PARAM,
				       MGMT_STATUS_INVALID_PARAMS);

etc...