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

[Malladi, Meghana]

On 3/28/2025 1:32 PM, Dan Carpenter wrote:
> On Fri, Mar 28, 2025 at 11: 46: 49AM +0530, Malladi, Meghana wrote: > > 
>  > On 3/25/2025 11: 18 PM, Jakub Kicinski wrote: > > On Fri, 21 Mar 2025 
> 13: 43: 13 +0530 Meghana Malladi wrote: > > > Whenever there is a perout 
> request
> 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! 
> uldqfVdF3CcxCkXE1pKJH1UXrlua- 
> GvI0DSoHvw1l5Yyu6xCbgDWX8PlhMSuV5lCv48fT7ChvXbQ105c6PHChn2lWCWZMsMcoHjvVwM$>
> ZjQcmQRYFpfptBannerEnd
>
> On Fri, Mar 28, 2025 at 11:46:49AM +0530, Malladi, Meghana wrote:
> > On 3/25/2025 11:18 PM, Jakub Kicinski wrote:
> > > On Fri, 21 Mar 2025 13:43:13 +0530 Meghana Malladi wrote:
> > > > Whenever there is a perout request from the user application,
> > > > kernel receives req structure containing the configuration info
> > > > for that req.
> > > 
> > > This doesn't really explain the condition under which the bug triggers.
> > > Presumably when user request comes in req is never NULL?
> > > 
> >
> > You are right, I have looked into what would trigger this bug but seems like
> > user request can never be NULL, but the contents inside the req can be
> > invalid, but that is already being handled by the kernel. So this bug fix
> > makes no sense and I will be dropping this patch for v3. Thanks.
>
> I don't remember bug reports for more than a few hours so I had to dig
> this up on lore:
>
> https://urldefense.com/v3/__https://lore.kernel.org/ 
> all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/__;!!G3vK! 
> XPLUWNuB49W9FXpnac95FM8thR9_zMOBwt7JgYy8Yaf72LIm4Xt-3Yc8h1sEti5uVdguSWQhcfsnC1_ymQIMTg$ <https://urldefense.com/v3/__https://lore.kernel.org/all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/__;!!G3vK!XPLUWNuB49W9FXpnac95FM8thR9_zMOBwt7JgYy8Yaf72LIm4Xt-3Yc8h1sEti5uVdguSWQhcfsnC1_ymQIMTg$>
>
> This is definitely still a real bug on today's linux-next but yes, the
> fix is bad.
>
> drivers/net/ethernet/ti/icssg/icss_iep.c
>     814  int icss_iep_exit(struct icss_iep *iep)
>     815  {
>     816          if (iep->ptp_clock) {
>     817                  ptp_clock_unregister(iep->ptp_clock);
>     818                  iep->ptp_clock = NULL;
>     819          }
>     820          icss_iep_disable(iep);
>     821
>     822          if (iep->pps_enabled)
>     823                  icss_iep_pps_enable(iep, false);
>     824          else if (iep->perout_enabled)
>     825                  icss_iep_perout_enable(iep, NULL, false);
>                                                      ^^^^
> A better fix probably to delete this function call instead of
> turning it into a no-op.

Yes agreed, current bug fix is bad. Will have a fix similar to this and 
post it soon. Thanks.

>     826
>     827          return 0;
>     828  }
>
> regards,
> dan carpenter