1. Patchew
  2. linux
  3. [PATCH net-next v2 0/3] Bug fixes from XDP and perout series
  4. View patch

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

Meghana Malladi posted 3 patches 9 months ago
There is a newer version of this series
[PATCH net-next v2 3/3] net: ti: icss-iep: Fix possible NULL pointer dereference for perout request
Posted by Meghana Malladi 9 months ago 
Whenever there is a perout request from the user application,
kernel receives req structure containing the configuration info
for that req. Add NULL pointer handling for perout request if
that req struct points to NULL.

Fixes: e5b456a14215 ("net: ti: icss-iep: Add pwidth configuration for perout signal")
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
Reviewed-by: Simon Horman <horms@kernel.org>
---

Changes from v1(v2-v1):
- Collected RB tag from Simon Horman <horms@kernel.org>

 drivers/net/ethernet/ti/icssg/icss_iep.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index b4a34c57b7b4..aeebdc4c121e 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -498,6 +498,10 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
 {
 	int ret = 0;
 
+	/* Return error if the req is NULL */
+	if (!req)
+		return -EINVAL;
+
 	/* Reject requests with unsupported flags */
 	if (req->flags & ~(PTP_PEROUT_DUTY_CYCLE |
 			  PTP_PEROUT_PHASE))
-- 
2.43.0
Re: [PATCH net-next v2 3/3] net: ti: icss-iep: Fix possible NULL pointer dereference for perout request
Posted by Jakub Kicinski 8 months, 3 weeks ago 
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?

> Add NULL pointer handling for perout request if
> that req struct points to NULL.
> 
> Fixes: e5b456a14215 ("net: ti: icss-iep: Add pwidth configuration for perout signal")
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> ---
> 
> Changes from v1(v2-v1):
> - Collected RB tag from Simon Horman <horms@kernel.org>
> 
>  drivers/net/ethernet/ti/icssg/icss_iep.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
> index b4a34c57b7b4..aeebdc4c121e 100644
> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
> @@ -498,6 +498,10 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
>  {
>  	int ret = 0;
>  
> +	/* Return error if the req is NULL */

code is trivial here, explain the 'why' not the 'what'
Why is this called with NULL?

> +	if (!req)
> +		return -EINVAL;
-- 
pw-bot: cr
Re: [PATCH net-next v2 3/3] net: ti: icss-iep: Fix possible NULL pointer dereference for perout request
Posted by Malladi, Meghana 8 months, 3 weeks ago 

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.

>> Add NULL pointer handling for perout request if
>> that req struct points to NULL.
>>
>> Fixes: e5b456a14215 ("net: ti: icss-iep: Add pwidth configuration for perout signal")
>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>> Reviewed-by: Simon Horman <horms@kernel.org>
>> ---
>>
>> Changes from v1(v2-v1):
>> - Collected RB tag from Simon Horman <horms@kernel.org>
>>
>>   drivers/net/ethernet/ti/icssg/icss_iep.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
>> index b4a34c57b7b4..aeebdc4c121e 100644
>> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
>> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
>> @@ -498,6 +498,10 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
>>   {
>>   	int ret = 0;
>>   
>> +	/* Return error if the req is NULL */
> 
> code is trivial here, explain the 'why' not the 'what'
> Why is this called with NULL?
> 
>> +	if (!req)
>> +		return -EINVAL;
Re: [PATCH net-next v2 3/3] net: ti: icss-iep: Fix possible NULL pointer dereference for perout request
Posted by Dan Carpenter 8 months, 3 weeks ago 
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://lore.kernel.org/all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/

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.

   826  
   827          return 0;
   828  }

regards,
dan carpenter
Re: [EXTERNAL] Re: [PATCH net-next v2 3/3] net: ti: icss-iep: Fix possible NULL pointer dereference for perout request
Posted by Malladi, Meghana 8 months, 3 weeks ago 

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
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.