From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from fhigh-a6-smtp.messagingengine.com (fhigh-a6-smtp.messagingengine.com [103.168.172.157]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4B0F625B0A2 for ; Sat, 25 Jul 2026 07:28:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=103.168.172.157 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784964521; cv=none; b=MIVN5hwkxCSMBoWmLwAChD2vP0jLr+qxygwUkK1uJfOe6B67Ks/pKyhmAgATs5ypyWwQpghv9nkIbKD3Q/SFBvkeSUfUhhcc4AtbYYodG3SZcg74KwlMOFBqNTndQY46xk3If8txyQE7iDMDYLWzpZv0iG4Mug79j3QGHb9Mq/4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784964521; c=relaxed/simple; bh=8DH946+V1xDpzzsjINJmUhetebNFwW1JqI4BKeftXOI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=bpwR0hsPu9l274FenQ1M5owFNGivYJEoFlW2xjUcTO/ISYeICaBGRVo/HN4Q3ChjoyoRO201V971VU0YyFbC0V7kOOYKtnG7eyHOfaYYJxfVi4uMSNQYU0Yd8ILjJmSLaSc3OMobusXd5FRRV/Uy7jSSPG+pNBkriO3YPW2sH44= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=sakamocchi.jp; spf=pass smtp.mailfrom=sakamocchi.jp; dkim=pass (2048-bit key) header.d=sakamocchi.jp header.i=@sakamocchi.jp header.b=QvLNggkt; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=ompC17Ov; arc=none smtp.client-ip=103.168.172.157 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=sakamocchi.jp Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=sakamocchi.jp Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=sakamocchi.jp header.i=@sakamocchi.jp header.b="QvLNggkt"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="ompC17Ov" Received: from phl-compute-05.internal (phl-compute-05.internal [10.202.2.45]) by mailfhigh.phl.internal (Postfix) with ESMTP id 3EB4F140022A; Sat, 25 Jul 2026 03:28:37 -0400 (EDT) Received: from phl-frontend-03 ([10.202.2.162]) by phl-compute-05.internal (MEProxy); Sat, 25 Jul 2026 03:28:37 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sakamocchi.jp; h=cc:cc:content-type:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:subject:subject:to:to; s=fm2; t=1784964517; x= 1785050917; bh=saMq8sTv6NeEM79g1cID5p6Zb0jLsF8FBF9xXNJRTFI=; b=Q vLNggktoxmHTOWmBvYRv7P7cicrBbhjmLb7ASgR+0SqDckLKSP9a+1XTWrusjMFU U7nDbDMkTxUQFnHlPLht4cV5RYw6rKxIaJKGHLMsL6XEpN8662EvHjr1OrxMCbsV OYEr70eKuX7y9ewo6XEYU0HNLzkQ15ZvIKrK9YmaVrlpM11SrQK6VxDBI+x+Xz5N i4SB/h+28E6zW45JDPYSrqtWRVuCfOjxDYFZUExLEJJLhWDsjpLsqsDTThm5lzxR ENg2qO0myTZ6ZLpDMPzOCFJD4Q4NGUAn8fLcJOjvdE+2UoQyRk52tcAP8CUjvZ3b PLQlty6KpnSiTIFWk1Lmg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t= 1784964517; x=1785050917; bh=saMq8sTv6NeEM79g1cID5p6Zb0jLsF8FBF9 xXNJRTFI=; b=ompC17OvO0DBSV4DCnytp2wcu8vf8X5ogbVM/HCbui7JdNcpJJ+ CIEtJVFEGRQsfWN7NM5+nkUCNMcpUV2mKMrPape+L09oUdv2hwD7W1ftoaG92uPv RZKzXc62srRUrGjM3TV4wnLam1RDqXvo5dy0GhWRDjYPw6X6M3rTGUHpnNKcs8UA iIhUWD8NFU8h+McJfP6v5qgsQy/uGCMEiZUmFh0EO4dGYHwAgXBqqucR4stAV4ia CNICcO2yKSgtxGdysRaHhm6pEVzTSaiCEykW0ndnhZH6axj2FYjpzEVM/+KqmVOY IlEqO8wPbyMeNsAQDpUXpYzUvNYKU466ZsA== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: dmFkZTECg0u6knwGzKaPkApKiGHV98OF10nBt7uPP29qWyDc5aNEtW7ADm3avzbJLe6B2A DOD+sBPMtQ881n3bUos3XVCxuxOrvr7XkQTqyAVYIK6hiKRfaJfc22NCKRre7JVOS5tRFh D8e3p6eDFEwKGQjmUcyTsgGA0zGjjxldZrwB8LrpRM+tcT6SdlNTkrZyce6Ld+k6YCKUNX Qed0+hD5cmw7IFbuRuF2dbmveAxpPqJlLq+QVvuc0aNdTyvD2Xu7MXqbInNQ1+BNPeaJdV r+wyplSOadx5zu10FE/LhiVvYXaEVsIZWApae2R9R4YMlWS9VS+Wp7w/G9fiswvsvY9cCK wDMH/iSnf+iHRj5PUYq1KfwE7QPv9FPfUWgxinnhtdhgS/Wv8x2k+98kMIZlqccL+DHBrD 3z+Gz+Q0tfnPz8uSqbO/OpeyIZf7j7MQrZ6Ow7rNrBxewpVOasz9qTU4URL7lTJXQB24om 3GZI6QcacsqajXsC20yxVFnoAiFbXpnNbj049e0kTEmpwvhc48GObqDP41vM6Boj2Pub+F 5QxO96xM6GriJXDBoZFG4AxVpnHAR2K+n/rYR1NOd3ll6K2rTEmYQDhIrCbDZQi9L1DDrx 9s5MmWtVrc8v7lk852yfh2/nIfh4rR/7RUjdAiWkl4mZXjHxDnBc7RnnHaqA X-ME-Proxy: Feedback-ID: ie8e14432:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sat, 25 Jul 2026 03:28:36 -0400 (EDT) Date: Sat, 25 Jul 2026 16:28:33 +0900 From: Takashi Sakamoto To: Sreeraj S Kurup Cc: linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] firewire: core: validate descriptor bounds in fw_core_add_descriptor() Message-ID: <20260725072833.GA46151@sakamocchi.jp> Mail-Followup-To: Sreeraj S Kurup , linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org References: <20260724101406.18673-1-sreekuttan2156239@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260724101406.18673-1-sreekuttan2156239@gmail.com> Hi, Sorry to be late for reply. On Fri, Jul 24, 2026 at 10:14:06AM +0000, Sreeraj S Kurup wrote: > In fw_core_add_descriptor(), incoming descriptor data is processed without > prior bounds verification of individual sub-block headers. A malformed or > corrupted descriptor containing an oversized block length field can cause > the parsing loop to read past the end of the desc->data buffer, leading to > an out-of-bounds memory access. > > Add validation logic at the start of fw_core_add_descriptor() to: > 1. Reject empty descriptors (length == 0) or descriptors exceeding the > maximum configuration ROM size (256 quadlets). > 2. Validate each sub-block header's embedded length against the remaining > descriptor length before advancing the loop pointer. > 3. Ensure the inner loop explicitly increments by (block_len + 1) quadlets > to prevent hangs or buffer overflows. > > Signed-off-by: Sreeraj S Kurup > --- > v2: > - Fixed tab indentation to 8-space tabs per kernel coding style. > - Added explicit loop increment (i += block_len + 1) to prevent kernel hangs. > - Isolated changes strictly to drivers/firewire/core-card.c. > - Added descriptor bounds validation in fw_core_add_descriptor(). > > drivers/firewire/core-card.c | 57 +++++++++++++++++++++--------------- > 1 file changed, 34 insertions(+), 23 deletions(-) This v2 patch conflicts on my tree (7.2-rc4). I guess that v2 was written on the tree to which v1 patch is applied, so my comments are provided to the squashed patch below. ======== 8< -------- diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index a754c6366b97..cb8ce491fe9d 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -143,7 +143,11 @@ static void generate_config_rom(struct fw_card *card, __be32 *config_rom) for (i = 0; i < j; i += length + 1) length = fw_compute_block_crc(config_rom + i); - WARN_ON(j != config_rom_length); + if (j != config_rom_length) { + pr_warn("FireWire ROM length mismatch: expected %zu, got %d\n", + config_rom_length, j); + config_rom_length = j; + } } static void update_config_roms(void) @@ -167,15 +171,29 @@ int fw_core_add_descriptor(struct fw_descriptor *desc) { size_t i; + /* Reject empty or oversized descriptors early */ + if (desc->length == 0 || desc->length > 256) For the above check, in_range() macro in include/linux/minmax.h is also available. + return -EINVAL; + i = 0; /* - * Check descriptor is valid; the length of all blocks in the - * descriptor has to add up to exactly the length of the - * block. + * Validate internal block structures within the descriptor. Each sub-block + * encodes its length in the top 16 bits of its header quadlet. */ - i = 0; - while (i < desc->length) - i += (desc->data[i] >> 16) + 1; + while (i < desc->length) { + u16 block_len = desc->data[i] >> 16; + + /* + * Guard against corrupted descriptors where an individual block length + * claims to extend past the allocated end of desc->data, avoiding + * out-of-bounds reads. + */ + if (block_len >= desc->length - i) + return -EINVAL; + + i += block_len + 1; + } + /* The sum of sub-block lengths must match total descriptor length */ if (i != desc->length) return -EINVAL; ======== 8< -------- I think the above changes could be split into two patches: * Overall length validation. * Descriptor length validation. Thanks Takashi Sakamoto