RE: [PATCH v3 1/3] Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus hardening

[Michael Kelley]

From: Andres Beltran <lkmlabelt@xxxxxxxxx> Sent: Tuesday, June 30, 2020 8:32 AM
> 
> +
> +/*
> + * vmbus_next_request_id - Returns a new request id. It is also
> + * the index at which the guest memory address is stored.
> + * Uses a spin lock to avoid race conditions.
> + * @rqstor: Pointer to the requestor struct
> + * @rqst_add: Guest memory address to be stored in the array
> + */
> +u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr)
> +{
> +	unsigned long flags;
> +	u64 current_id;
> +
> +	/* Check rqstor has been initialized */
> +	if (!rqstor->size)
> +		return VMBUS_RQST_ERROR;

Conceptually, this check isn't really correct.  If the rqstor structure
hasn't been initialized, then the value of the "size" field is also
uninitialized and hence might or might not be zero.  You would
have to check the rqstor_size field in the channel struct to
correctly determine if the rqstor structure was initialized.

Because the rqstor structure is embedded in the channel struct, and
the channel struct is initialized to all zeroes, this test works.  But
it could break if the rqstor structure was allocated elsewhere
and wasn't initialized to all zeros.

> +
> +	spin_lock_irqsave(&rqstor->req_lock, flags);
> +	current_id = rqstor->next_request_id;
> +
> +	/* Requestor array is full */
> +	if (current_id >= rqstor->size) {
> +		current_id = VMBUS_RQST_ERROR;
> +		goto exit;
> +	}
> +
> +	rqstor->next_request_id = rqstor->req_arr[current_id];
> +	rqstor->req_arr[current_id] = rqst_addr;
> +
> +	/* The already held spin lock provides atomicity */
> +	bitmap_set(rqstor->req_bitmap, current_id, 1);
> +
> +exit:
> +	spin_unlock_irqrestore(&rqstor->req_lock, flags);
> +	return current_id;
> +}
> +EXPORT_SYMBOL_GPL(vmbus_next_request_id);
> +
> +/*
> + * vmbus_request_addr - Returns the memory address stored at @trans_id
> + * in @rqstor. Uses a spin lock to avoid race conditions.
> + * @rqstor: Pointer to the requestor struct
> + * @trans_id: Request id sent back from Hyper-V. Becomes the requestor's
> + * next request id.
> + */
> +u64 vmbus_request_addr(struct vmbus_requestor *rqstor, u64 trans_id)
> +{
> +	unsigned long flags;
> +	u64 req_addr;
> +
> +	/* Check rqstor has been initialized */
> +	if (!rqstor->size)
> +		return VMBUS_RQST_ERROR;

Same problem here.

Michael

> +
> +	spin_lock_irqsave(&rqstor->req_lock, flags);
> +
> +	/* Invalid trans_id */
> +	if (trans_id >= rqstor->size) {
> +		req_addr = VMBUS_RQST_ERROR;
> +		goto exit;
> +	}
> +
> +	/* Invalid trans_id: empty slot */
> +	if (!test_bit(trans_id, rqstor->req_bitmap)) {
> +		req_addr = VMBUS_RQST_ERROR;
> +		goto exit;
> +	}
> +
> +	req_addr = rqstor->req_arr[trans_id];
> +	rqstor->req_arr[trans_id] = rqstor->next_request_id;
> +	rqstor->next_request_id = trans_id;
> +
> +	/* The already held spin lock provides atomicity */
> +	bitmap_clear(rqstor->req_bitmap, trans_id, 1);
> +
> +exit:
> +	spin_unlock_irqrestore(&rqstor->req_lock, flags);
> +	return req_addr;
> +}
> +EXPORT_SYMBOL_GPL(vmbus_request_addr);