Re: [PATCH] firewire: core: validate response length to prevent buffer overflow

Hi,

Sorry to be late for reply, but I always postpone patch review during
merge window. Now 6.19-rc1 has been released, and we can start working
to fix it.

On Wed, Dec 03, 2025 at 10:22:32AM +0800, Junrui Luo wrote:
> The FireWire core transaction handling code does not validate that
> the length of a READ_BLOCK_RESPONSE matches the length originally
> requested in the READ_BLOCK_REQUEST. A malicious FireWire device
> could respond with more data than requested, causing a buffer overflow
> in the callback handler when the response data is copied into the
> caller's buffer.
> 
> This issue has been acknowledged by a FIXME comment:
>   "FIXME: sanity check packet, is length correct, does tcodes
>    and addresses match to the transaction request queried later."
> 
> Fix this by validating the response length against the original request
> length before passing data to the callback.
> 
> Reported-by: Yuhao Jiang <[email protected]>
> Reported-by: Junrui Luo <[email protected]>
> Fixes: 3038e353cfaf ("firewire: Add core firewire stack.")
> Signed-off-by: Junrui Luo <[email protected]>
> ---
>  drivers/firewire/core-transaction.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
 
Thanks for your trial to fix the TODO, however I can still find an issue
in your patch.

> diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
> index c65f491c54d0..52f05e8f3798 100644
> --- a/drivers/firewire/core-transaction.c
> +++ b/drivers/firewire/core-transaction.c
> @@ -1095,11 +1095,23 @@ void fw_core_handle_request(struct fw_card *card, struct fw_packet *p)
>  }
>  EXPORT_SYMBOL(fw_core_handle_request);
>  
> +static size_t get_request_data_length(const struct fw_packet *request)
> +{
> +	int request_tcode = async_header_get_tcode(request->header);
> +
> +	if (request_tcode == TCODE_READ_QUADLET_REQUEST)
> +		return 4;
> +	else if (request_tcode == TCODE_READ_BLOCK_REQUEST)
> +		return async_header_get_data_length(request->header);
> +	return 0;
> +}
> +

The response for lock transaction request can include data. We need to
check in this case here.

>  void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
>  {
>  	struct fw_transaction *t = NULL, *iter;
>  	u32 *data;
>  	size_t data_length;
> +	size_t request_length;
>  	int tcode, tlabel, source, rcode;
>  
>  	tcode = async_header_get_tcode(p->header);
> @@ -1107,9 +1119,6 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
>  	source = async_header_get_source(p->header);
>  	rcode = async_header_get_rcode(p->header);
>  
> -	// FIXME: sanity check packet, is length correct, does tcodes
> -	// and addresses match to the transaction request queried later.
> -	//
>  	// For the tracepoints event, let us decode the header here against the concern.
>  
>  	switch (tcode) {
> @@ -1160,6 +1169,13 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
>  		return;
>  	}
>  
> +	request_length = get_request_data_length(&t->packet);
> +	if (request_length > 0 && data_length > request_length) {
> +		fw_notice(card, "response length (%zu) exceeds request length (%zu) from node %x, truncating\n",
> +			  data_length, request_length, source);
> +		data_length = request_length;
> +	}
> +
>  	/*
>  	 * The response handler may be executed while the request handler
>  	 * is still pending.  Cancel the request handler.


Thanks

Takashi Sakamoto