netdev - Re: [PATCH net 5/6] igc: Fix launchtime before start of cycle

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]

Date: Tue, 11 Jul 2023 10:37:48 +0200
From: Florian Kauer <florian.kauer@...utronix.de>
To: Leon Romanovsky <leon@...nel.org>,
 Tony Nguyen <anthony.l.nguyen@...el.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, netdev@...r.kernel.org, kurt@...utronix.de,
 vinicius.gomes@...el.com, muhammad.husaini.zulkifli@...el.com,
 tee.min.tan@...ux.intel.com, aravindhan.gunasekaran@...el.com,
 sasha.neftin@...el.com, Naama Meir <naamax.meir@...ux.intel.com>
Subject: Re: [PATCH net 5/6] igc: Fix launchtime before start of cycle

On 11.07.23 09:09, Leon Romanovsky wrote:
> On Mon, Jul 10, 2023 at 09:35:02AM -0700, Tony Nguyen wrote:
>> From: Florian Kauer <florian.kauer@...utronix.de>
>>
>> It is possible (verified on a running system) that frames are processed
>> by igc_tx_launchtime with a txtime before the start of the cycle
>> (baset_est).
>>
>> However, the result of txtime - baset_est is written into a u32,
>> leading to a wrap around to a positive number. The following
>> launchtime > 0 check will only branch to executing launchtime = 0
>> if launchtime is already 0.
>>
>> Fix it by using a s32 before checking launchtime > 0.
>>
>> Fixes: db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag bit")
>> Signed-off-by: Florian Kauer <florian.kauer@...utronix.de>
>> Reviewed-by: Kurt Kanzenbach <kurt@...utronix.de>
>> Tested-by: Naama Meir <naamax.meir@...ux.intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>> ---
>>  drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 5d24930fed8f..4855caa3bae4 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -1016,7 +1016,7 @@ static __le32 igc_tx_launchtime(struct igc_ring *ring, ktime_t txtime,
>>  	ktime_t base_time = adapter->base_time;
>>  	ktime_t now = ktime_get_clocktai();
>>  	ktime_t baset_est, end_of_cycle;
>> -	u32 launchtime;
>> +	s32 launchtime;
> 
> The rest of igc_tx_launchtime() function is very questionable,
> as ktime_sub_ns() returns ktime_t which is s64.
> 
>   1049         launchtime = ktime_sub_ns(txtime, baset_est);
>   1050         if (launchtime > 0)
>   1051                 div_s64_rem(launchtime, cycle_time, &launchtime);
>   1052         else
>   1053                 launchtime = 0;
>   1054
>   1055         return cpu_to_le32(launchtime);
> 

If I understand correctly, ktime_sub_ns(txtime, baset_est) will only return
something larger than s32 max if cycle_time is larger than s32 max and if that
is the case everything will be broken anyway since the corresponding hardware
register only holds 30 bits.

However, I do not see on first inspection where that case is properly handled
(probably just by rejecting the TAPRIO schedule).

Can someone with more experience in that area please jump in?

Thanks,
Florian

> 
>>  	s64 n;
>>  
>>  	n = div64_s64(ktime_sub_ns(now, base_time), cycle_time);
>> -- 
>> 2.38.1
>>
>>