Re: [EXTERNAL] Re: [PATCH net v3 3/3] net: ti: icss-iep: Fix possible NULL pointer dereference for perout request

[Malladi, Meghana]

On 4/3/2025 4:55 PM, Paolo Abeni wrote:
> On 4/2/25 2: 37 PM, Malladi, Meghana wrote: > On 4/2/2025 5: 58 PM, 
> Roger Quadros wrote: >> On 28/03/2025 12: 24, Meghana Malladi wrote: >>> 
> ICSS IEP driver has flags to check if perout or pps has been enabled >>> at
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source 
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK! 
> updqndalvOwRqgXOXDPJf9wy4vKojW68gavBCIsz5DlBLvSeawwT53qgFGcvIm0ULRBQkJv028AcR194Ei9ZDPp5ily-uAw$>
> ZjQcmQRYFpfptBannerEnd
>
> On 4/2/25 2:37 PM, Malladi, Meghana wrote:
> > On 4/2/2025 5:58 PM, Roger Quadros wrote:
> > > On 28/03/2025 12:24, Meghana Malladi wrote:
> > > > ICSS IEP driver has flags to check if perout or pps has been enabled
> > > > at any given point of time. Whenever there is request to enable or
> > > > disable the signal, the driver first checks its enabled or disabled
> > > > and acts accordingly.
> > > >
> > > > After bringing the interface down and up, calling PPS/perout enable
> > > > doesn't work as the driver believes PPS is already enabled,
> > >
> > > How? aren't we calling icss_iep_pps_enable(iep, false)?
> > > wouldn't this disable the IEP and clear the iep->pps_enabled flag?
> >
> > The whole purpose of calling icss_iep_pps_enable(iep, false) is to clear 
> > iep->pps_enabled flag, because if this flag is not cleared it hinders 
> > generating new pps signal (with the newly loaded firmware) once any of 
> > the interfaces are up (check icss_iep_perout_enable()), so instead of 
> > calling icss_iep_pps_enable(iep, false) I am just clearing the 
> > iep->pps_enabled flag.
>
> IDK what Roger thinks, but the above is not clear to me. I read it as
> you are stating that icss_iep_pps_enable() indeed clears the flag, so i
> don't see/follow the reasoning behind this change.

The reason behind this change is to fix the possible null pointer 
dereference which will occur when icss_iep_perout_enable(iep, NULL, 
false) is called during icss_iep_exit(), my bad for not mentioning it 
clearly in the commit message.

> Skimmir over the code, icss_iep_pps_enable() could indeed avoid clearing
> the flag under some circumstances is that the reason?

icss_iep_pps_enable() does indeed clear the flag, but 
icss_iep_perout_enable() doesn't due to the null pointer dereference. So 
instead of calling these functions for clearing the flag, we can simply 
just clear the flag directly.

> Possibly a more describing commit message would help.

Yes agreed. I will update it for v4.

> > > And, what if you brought 2 interfaces of the same ICSS instances up
> > > but put only 1 interface down and up?
> >
> > Then the flag need not be disabled if only one interface is brought down 
> > because the IEP is still alive and all the IEP configuration (including 
> > pps/perout) is still valid.
>
> I read the above as stating this fix is not correct in such scenario,
> leading to the wrong final state.

No it is the correct scenario. When you bring down all the existing 
interfaces (be it one or two, when whatever is up goes down) you unload 
the existing firmware and clear the all the driver configuration (this 
flag also needs to be cleared) to ensure everything starts with a clean 
state.

> I think it would be better to either give a better reasoning about this
> change in the commit message or refactor it to handle even such scenario,
>
> Thanks,
>
> Paolo