netdev - RE: [PATCH iwl-net v1] ixgbe: fix ixgbe_orom_civd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <PH0PR11MB5902E977BFC90DCC8DB99FD9F027A@PH0PR11MB5902.namprd11.prod.outlook.com>
Date: Thu, 31 Jul 2025 11:04:42 +0000
From: "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>
To: Simon Horman <horms@...nel.org>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "Loktionov, Aleksandr"
	<aleksandr.loktionov@...el.com>
Subject: RE: [PATCH iwl-net v1] ixgbe: fix ixgbe_orom_civd_info struct layout

From: Simon Horman <horms@...nel.org> 
Sent: Wednesday, July 30, 2025 5:54 PM

>On Wed, Jul 30, 2025 at 04:52:09PM +0200, Jedrzej Jagielski wrote:
>> The current layout of struct ixgbe_orom_civd_info causes incorrect data
>> storage due to compiler-inserted padding. This results in issues when
>> writing OROM data into the structure.
>> 
>> Add the __packed attribute to ensure the structure layout matches the
>> expected binary format without padding.
>> 
>> Fixes: 70db0788a262 ("ixgbe: read the OROM version information")
>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>> ---
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> index 09df67f03cf4..38a41d81de0f 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> @@ -1150,7 +1150,7 @@ struct ixgbe_orom_civd_info {
>>  	__le32 combo_ver;	/* Combo Image Version number */
>>  	u8 combo_name_len;	/* Length of the unicode combo image version string, max of 32 */
>>  	__le16 combo_name[32];	/* Unicode string representing the Combo Image version */
>> -};
>> +} __packed;
>
>...
>
>Hi Jedrzej,
>
>I agree that this is correct. Otherwise there will be a 3 byte hole before
>combo_ver and a 1 byte hole before and combo_name. Which, based on the
>commit message, I assume is not part of the layout this structure
>represents.
>
>A side effect of this change is that both combo_ver and elements of
>combo_name become unaligned. As elements combo_ver does not seem to be
>accessed directly, things seem clean there. But in the case of combo_ver,
>I wonder if this change is also needed. (Compile tested only!)

Hi Simon

it's definitely worth incorporating into the patch, due to risk of unaligmnent.

Thanks for your suggestion!

>
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>index 71ea25de1bac..754c176fd4a7 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>@@ -3123,7 +3123,7 @@ static int ixgbe_get_orom_ver_info(struct ixgbe_hw *hw,
> 	if (err)
> 		return err;
> 
>-	combo_ver = le32_to_cpu(civd.combo_ver);
>+	combo_ver = get_unaligned_le32(&civd.combo_ver);
> 
> 	orom->major = (u8)FIELD_GET(IXGBE_OROM_VER_MASK, combo_ver);
> 	orom->patch = (u8)FIELD_GET(IXGBE_OROM_VER_PATCH_MASK, combo_ver);
>