netdev - Re: [PATCH net] ice: fix unaligned access in ice_create_lag

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: <8431dc3b-fdfa-4b4d-91cc-ab64c8e93f6c@intel.com>
Date: Thu, 9 Jan 2025 16:56:13 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
CC: Hongchen Zhang <zhanghongchen@...ngson.cn>, Michal Schmidt
	<mschmidt@...hat.com>, Dave Ertman <david.m.ertman@...el.com>,
	<netdev@...r.kernel.org>, Jesse Brandeburg <jesse.brandeburg@...el.com>,
	"Tony Nguyen" <anthony.l.nguyen@...el.com>, Daniel Machon
	<daniel.machon@...rochip.com>, <intel-wired-lan@...ts.osuosl.org>
Subject: Re: [PATCH net] ice: fix unaligned access in ice_create_lag_recipe

From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Date: Thu, 9 Jan 2025 10:31:35 +0100

> On 1/9/25 02:54, Hongchen Zhang wrote:
>> Hi Przemek,
>> On 2025/1/8 下午4:59, Przemek Kitszel wrote:
>>> On 1/8/25 04:09, Hongchen Zhang wrote:
>>
>>>> Hi Michal,
>>>> On 2024/1/31 pm 7:58, Michal Schmidt wrote:
>>>>> new_rcp->recipe_bitmap was written to as if it were an aligned bitmap.
>>>>> It is an 8-byte array, but aligned only to 4.
>>>>> Use put_unaligned to set its value.
>>>>>
>>>>> Additionally, values in ice commands are typically in little-endian.
>>>>> I assume the recipe bitmap should be too, so use the *_le64
>>>>> conversion.
>>>>> I don't have a big-endian system with ice to test this.
>>>>>
>>>>> I tested that the driver does not crash when probing on aarch64
>>>>> anymore,
>>>>> which is good enough for me. I don't know if the LAG feature actually
>>>>> works.
>>>>>
>>>>> This is what the crash looked like without the fix:
>>>
>>>>> [   17.599142] Call trace:
>>>>> [   17.599143]  ice_create_lag_recipe.constprop.0+0xbc/0x11c [ice]
>>>>> [   17.599172]  ice_init_lag+0xcc/0x22c [ice]
>>>>> [   17.599201]  ice_init_features+0x160/0x2b4 [ice]
>>>>> [   17.599230]  ice_probe+0x2d0/0x30c [ice]
>>>>> [   17.599258]  local_pci_probe+0x58/0xb0
>>>>> [   17.599262]  work_for_cpu_fn+0x20/0x30
>>>
>>>> I encountered the same problem on a LoongArch LS3C6000 machine. Can
>>>> this patch be merged now?
>>>
>>> What kernel base do you use?, we have merged the Steven Patches long ago
>> My test is based on 6.6.61 which contains Steven's patch:
>>   8ec08ba97fab 2024-05-07  ice: Refactor FW data type and fix bitmap
>> casting issue [Steven Zou]
>>
>> It seems that Steven's patch can not solve the unaligned access
>> problem caused by new_rcp->recipe_bitmap, So is Michal's patch (may
>> need some change in ice_add_sw_recipe()) still needed?
>>
> 
> thank you, I see now
> 
> I agree that ice_aqc_recipe_data_elem::recipe_bitmap[8] should be
> changed to __le64, together with updated accesses. Best way to do so

Too bad I didn't notice that in ice_create_lag_recipe(), the cast is
still here. It's not valid to cast 1-byte array to a naturally aligned
one (of unsigned longs).

You can't simply change it to __le64 as 8-byte vars are
naturally-aligned, while here its offset is 4 bytes.

The best solution would be to change it to two __le32s and avoid using
bitmap helpers like set_bit() at all -- just manually assign bits there.
Alternatively, if you want -- you can use __le64, but then pack the
structure by 4 bytes, but I don't think it would give any benefit
comparing to the former.

> will be as in Steven's patch.
> 
> @Michal, will you be OK with us reimplementing this, or you want to
> follow up?

Thanks,
Olek