[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <52d96554-23f6-4bde-84d9-e0a7a40b0bc3@bell-sw.com>
Date: Mon, 26 May 2025 08:42:30 +0000
From: Alexey Kodanev <aleksei.kodanev@...l-sw.com>
To: Rengarajan.S@...rochip.com, netdev@...r.kernel.org
Cc: andrew+netdev@...n.ch, Bryan.Whitehead@...rochip.com,
davem@...emloft.net, Raju.Lakkaraju@...rochip.com, pabeni@...hat.com,
kuba@...nel.org, edumazet@...gle.com, UNGLinuxDriver@...rochip.com,
richardcochran@...il.com
Subject: Re: [PATCH net-next] net: lan743x: fix 'channel' index check before
writing ptp->extts[]
Hi Rengarajan!
On 23.05.2025 20:21, Rengarajan.S@...rochip.com wrote:
> Hi Alexey,
>
> On Thu, 2025-05-22 at 14:13 +0000, Alexey Kodanev wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>> know the content is safe
>>
>> Before calling lan743x_ptp_io_event_clock_get(), the 'channel' value
>> is checked against the maximum value of
>> PCI11X1X_PTP_IO_MAX_CHANNELS(8).
>> This seems correct at first and aligns with the PTP interrupt status
>> register (PTP_INT_STS) specifications.
>>
>> However, lan743x_ptp_io_event_clock_get() writes to ptp->extts[] with
>> only LAN743X_PTP_N_EXTTS(4) elements, using channel as an index:
>>
>> lan743x_ptp_io_event_clock_get(..., u8 channel,...)
>> {
>> ...
>> /* Update Local timestamp */
>> extts = &ptp->extts[channel];
>> extts->ts.tv_sec = sec;
>> ...
>> }
>>
>
> As per the PTP_INT_STS definition, there are 8 sets of capture
> registers that can be configured as GPIO inputs. However, using
> LAN743X_PTP_N_EXTTS (4) restricts processing to only 4 GPIOs. Would it
> be more appropriate to update LAN743X_PTP_N_EXTTS to 8? This would
> ensure that extts = &ptp->extts[channel]; remains valid for all 8
> potential channel indices.
>
Yes, thought about the same, but I'm not sure why it was initially
limited to using only 4 channels in LAN743X_PTP_N_EXTTS.
Would it be okay to add the following to the patch?
--- a/drivers/net/ethernet/microchip/lan743x_ptp.h
+++ b/drivers/net/ethernet/microchip/lan743x_ptp.h
@@ -18,9 +18,8 @@
*/
#define LAN743X_PTP_N_EVENT_CHAN 2
#define LAN743X_PTP_N_PEROUT LAN743X_PTP_N_EVENT_CHAN
-#define LAN743X_PTP_N_EXTTS 4
+#define LAN743X_PTP_N_EXTTS 8 /* supports 8 GPIOs */
#define LAN743X_PTP_N_PPS 0
-#define PCI11X1X_PTP_IO_MAX_CHANNELS 8
#define PTP_CMD_CTL_TIMEOUT_CNT 50
>> To avoid a potential out-of-bounds write, let's use the maximum
>> value actually defined for the timestamp array to ensure valid
>> access to ptp->extts[channel] within its actual bounds.
>>
>> Detected using the static analysis tool - Svace.
>> Fixes: 60942c397af6 ("net: lan743x: Add support for PTP-IO Event
>> Input External Timestamp (extts)")
>> Signed-off-by: Alexey Kodanev <aleksei.kodanev@...l-sw.com>
>> ---
>>
>> Note that PCI11X1X_PTP_IO_MAX_CHANNELS will be unused after this
>> patch.
>> Could it perhaps be used to define LAN743X_PTP_N_EXTTS to support
>> size 8?
>>
>> drivers/net/ethernet/microchip/lan743x_ptp.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/microchip/lan743x_ptp.c
>> b/drivers/net/ethernet/microchip/lan743x_ptp.c
>> index 0be44dcb3393..1ef7978e768b 100644
>> --- a/drivers/net/ethernet/microchip/lan743x_ptp.c
>> +++ b/drivers/net/ethernet/microchip/lan743x_ptp.c
>> @@ -1121,7 +1121,7 @@ static long lan743x_ptpci_do_aux_work(struct
>> ptp_clock_info *ptpci)
>> PTP_INT_IO_FE
>> _MASK_) >>
>> PTP_INT_IO_FE
>> _SHIFT_);
>> if (channel >= 0 &&
>> - channel <
>> PCI11X1X_PTP_IO_MAX_CHANNELS) {
>> + channel < LAN743X_PTP_N_EXTTS) {
>> lan743x_ptp_io_event_clock_ge
>> t(adapter,
>>
>> true,
>>
>> channel,
>> @@ -1154,7 +1154,7 @@ static long lan743x_ptpci_do_aux_work(struct
>> ptp_clock_info *ptpci)
>> PTP_INT_IO_RE_
>> MASK_) >>
>> PTP_INT_IO_RE_
>> SHIFT_);
>> if (channel >= 0 &&
>> - channel <
>> PCI11X1X_PTP_IO_MAX_CHANNELS) {
>> + channel < LAN743X_PTP_N_EXTTS) {
>> lan743x_ptp_io_event_clock_ge
>> t(adapter,
>>
>> false,
>>
>> channel,
>> --
>> 2.25.1
>>
Powered by blists - more mailing lists