Re: [PATCH 1/9] dpp: prep for moving AAD within dpp_append_wrapped_data

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 3/27/24 10:19, James Prestwood wrote:
> The AAD pointers for DPP are specific to the frame type. This is
> currently sorted out by the caller within the respective frame
> building functions but its quite unreadable. There are some comments
> but lots of magic numbers. This should be moved within the
> dpp_append_wrapped_data utility but the first step is to make the
> frame buffer continuous. This will allow the entire frame to be

continuous -> contiguous?

> passed and dpp_append_wrapped_data can calculate the AAD offsets
> itself.

I'm a bit confused as to why?  Whether we have an iov or a contiguous buffer, 
the end result is the same, no?  If you want to omit parts of the header from 
the wrapped data calculation, wouldn't you just break up the header into 
multiple iovs instead?

> ---
>   src/dpp.c | 239 +++++++++++++++++++++++++-----------------------------
>   1 file changed, 112 insertions(+), 127 deletions(-)
> 

<snip>

> diff --git a/src/dpp.c b/src/dpp.c
> index 567fe8d2..5aac22a7 100644
> --- a/src/dpp.c
> +++ b/src/dpp.c
> @@ -648,7 +648,7 @@ static void dpp_frame_retry(struct dpp_sm *dpp)
>   
>   static size_t dpp_build_header(const uint8_t *src, const uint8_t *dest,
>   				enum dpp_frame_type type,
> -				uint8_t buf[static 32])
> +				uint8_t *buf)

Is this really needed?

>   {
>   	uint8_t *ptr = buf + 24;
>   
> @@ -672,7 +672,7 @@ static size_t dpp_build_header(const uint8_t *src, const uint8_t *dest,
>   
>   static size_t dpp_build_config_header(const uint8_t *src, const uint8_t *dest,
>   					uint8_t diag_token,
> -					uint8_t buf[static 37])
> +					uint8_t *buf)

Or this?

>   {
>   	uint8_t *ptr = buf + 24;
>   

<snip>

> @@ -780,42 +779,39 @@ static void dpp_configuration_start(struct dpp_sm *dpp, const uint8_t *addr)
>   	 * In this case there is no query request/response fields, nor any
>   	 * attributes besides wrapped data meaning zero AD components.
>   	 */
> -	ptr += dpp_append_wrapped_data(NULL, 0, NULL, 0, ptr, sizeof(attrs),
> +	ptr += dpp_append_wrapped_data(NULL, 0, NULL, 0, ptr, sizeof(frame),

The sizeof(frame) is now likely wrong in this setup.  No error checking also 
worries me a bit.

<snip>

>   static void send_config_result(struct dpp_sm *dpp, const uint8_t *to)
>   {
> -	uint8_t hdr[32];
> -	struct iovec iov[2];
> -	uint8_t attrs[256];
> -	uint8_t *ptr = attrs;
> +	struct iovec iov;
> +	uint8_t frame[256];
> +	uint8_t *ptr = frame;
>   	uint8_t zero = 0;
>   
> -	iov[0].iov_len = dpp_build_header(netdev_get_address(dpp->netdev), to,
> -					DPP_FRAME_CONFIGURATION_RESULT, hdr);
> -	iov[0].iov_base = hdr;
> -
> -	ptr += dpp_append_wrapped_data(hdr + 26, 6, attrs, 0, ptr,
> -			sizeof(attrs), dpp->ke, dpp->key_len, 2,
> +	ptr += dpp_build_header(netdev_get_address(dpp->netdev), to,
> +					DPP_FRAME_CONFIGURATION_RESULT, ptr);
> +	ptr += dpp_append_wrapped_data(frame + 26, 6, ptr, 0, ptr,
> +			sizeof(frame), dpp->ke, dpp->key_len, 2,

Same comment here

>   			DPP_ATTR_STATUS, (size_t) 1, &zero,
>   			DPP_ATTR_ENROLLEE_NONCE, dpp->nonce_len, dpp->e_nonce);
>   
> -	iov[1].iov_base = attrs;
> -	iov[1].iov_len = ptr - attrs;
> +	iov.iov_base = frame;
> +	iov.iov_len = ptr - frame;
>   
> -	dpp_send_frame(dpp, iov, 2, dpp->current_freq);
> +	dpp_send_frame(dpp, &iov, 1, dpp->current_freq);
>   }
>   
>   static void dpp_write_config(struct dpp_configuration *config,

<snip>

> @@ -1211,26 +1205,26 @@ static void dpp_send_config_response(struct dpp_sm *dpp, uint8_t status)
>   		json = dpp_configuration_to_json(dpp->config);
>   		json_len = strlen(json);
>   
> -		ptr += dpp_append_wrapped_data(attrs + 2, ptr - attrs - 2,
> -						NULL, 0, ptr, sizeof(attrs),
> +		ptr += dpp_append_wrapped_data(lptr + 2, ptr - lptr - 2,
> +						NULL, 0, ptr, sizeof(frame),

Is sizeof(frame) correct here?

>   						dpp->ke, dpp->key_len, 2,
>   						DPP_ATTR_ENROLLEE_NONCE,
>   						dpp->nonce_len, dpp->e_nonce,
>   						DPP_ATTR_CONFIGURATION_OBJECT,
>   						json_len, json);
>   	} else
> -		ptr += dpp_append_wrapped_data(attrs + 2, ptr - attrs - 2,
> -						NULL, 0, ptr, sizeof(attrs),
> +		ptr += dpp_append_wrapped_data(lptr + 2, ptr - lptr - 2,
> +						NULL, 0, ptr, sizeof(frame),

and here?

>   						dpp->ke, dpp->key_len, 2,
>   						DPP_ATTR_ENROLLEE_NONCE,
>   						dpp->nonce_len, dpp->e_nonce);
>   
> -	l_put_le16(ptr - attrs - 2, attrs);
> +	l_put_le16(ptr - lptr - 2, lptr);
>   
> -	iov[1].iov_base = attrs;
> -	iov[1].iov_len = ptr - attrs;
> +	iov.iov_base = frame;
> +	iov.iov_len = ptr - frame;
>   
> -	dpp_send_frame(dpp, iov, 2, dpp->current_freq);
> +	dpp_send_frame(dpp, &iov, 1, dpp->current_freq);
>   }
>   
>   static bool dpp_check_config_header(const uint8_t *ptr)

<snip>

Regards,
-Denis