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

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

[Junrui Luo]

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 <danisjiang@gmail.com>
Reported-by: Junrui Luo <moonafterrain@outlook.com>
Fixes: 3038e353cfaf ("firewire: Add core firewire stack.")
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/firewire/core-transaction.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

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;
+}
+
 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.

---
base-commit: 4a26e7032d7d57c998598c08a034872d6f0d3945
change-id: 20251203-fixes-760722565563

Best regards,
-- 
Junrui Luo <moonafterrain@outlook.com>

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

[Takashi Sakamoto]

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 <danisjiang@gmail.com>
> Reported-by: Junrui Luo <moonafterrain@outlook.com>
> Fixes: 3038e353cfaf ("firewire: Add core firewire stack.")
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
> ---
>  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