netdev - Re: [PATCH net-next] r8169: remove not needed check in rtl_fw_write

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <65832d7e-2880-4883-92b9-033e48c24d25@gmail.com>
Date: Thu, 23 Nov 2023 16:12:59 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Simon Horman <horms@...nel.org>
Cc: Realtek linux nic maintainers <nic_swsd@...ltek.com>,
 Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
 Eric Dumazet <edumazet@...gle.com>, David Miller <davem@...emloft.net>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] r8169: remove not needed check in
 rtl_fw_write_firmware

On 23.11.2023 15:54, Simon Horman wrote:
> On Thu, Nov 23, 2023 at 10:53:26AM +0100, Heiner Kallweit wrote:
>> This check can never be true for a firmware file with a correct format.
>> Existing checks in rtl_fw_data_ok() are sufficient, no problems with
>> invalid firmware files are known.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>>  drivers/net/ethernet/realtek/r8169_firmware.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169_firmware.c b/drivers/net/ethernet/realtek/r8169_firmware.c
>> index cbc6b846d..ed6e721b1 100644
>> --- a/drivers/net/ethernet/realtek/r8169_firmware.c
>> +++ b/drivers/net/ethernet/realtek/r8169_firmware.c
>> @@ -151,9 +151,6 @@ void rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw)
>>  		u32 regno = (action & 0x0fff0000) >> 16;
>>  		enum rtl_fw_opcode opcode = action >> 28;
>>  
>> -		if (!action)
>> -			break;
>> -
> 
> Hi Heiner,
> 
> I could well be wrong, but this does seem to guard against the following case:
> 
> 1. data = 0
> 2. regno = 0
> 3. opcode = 0 (PHY_READ)
> 
> Which does not seem to be checked in rtl_fw_data_ok().
> 
> It's unclear to me if there is any value in this guard.
> 
Value 0 is used with a special meaning in two places:
1. Newer firmwares with some meta data before the actual firmware
   have first dword 0 to be able to differentiate old and new fw format.
2. Typically (not always) fw files in new format have a trailing dword 0.

A potential problem (as you mention) is that value 0 isn't really a
sentinel value because reading PHY register 0 is a valid command.
It's just never used in their firmwares.

There's no need to guard from reading PHY reg 0. It does no harm.
I *think* they once added this check to detect end of file.
But that's not needed because the actual firmware length is
part of the meta data. Therefore reading data from the firmware
will stop before reaching the training zero(s).

>>  		switch (opcode) {
>>  		case PHY_READ:
>>  			predata = fw_read(tp, regno);
>> -- 
>> 2.43.0
>>