[PATCH net 1/1] i40e: fix PTP pins verification

[PATCH net 1/1] i40e: fix PTP pins verification

[Tony Nguyen]

From: Andrii Staikov <andrii.staikov@intel.com>

Fix PTP pins verification not to contain tainted arguments. As a new PTP
pins configuration is provided by a user, it may contain tainted
arguments that are out of bounds for the list of possible values that can
lead to a potential security threat. Change pin's state name from 'invalid'
to 'empty' for more clarification.

Fixes: 1050713026a0 ("i40e: add support for PTP external synchronization clock")
Signed-off-by: Andrii Staikov <andrii.staikov@intel.com>
Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_ptp.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
index c37abbb3cd06..78e7c705cd89 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
@@ -49,7 +49,7 @@ static struct ptp_pin_desc sdp_desc[] = {
 
 enum i40e_ptp_gpio_pin_state {
 	end = -2,
-	invalid,
+	empty,
 	off,
 	in_A,
 	in_B,
@@ -1078,11 +1078,19 @@ static int i40e_ptp_set_pins(struct i40e_pf *pf,
 	else if (pin_caps == CAN_DO_PINS)
 		return 0;
 
-	if (pins->sdp3_2 == invalid)
+	if ((pins->sdp3_2 < empty || pins->sdp3_2 > out_B) ||
+	    (pins->sdp3_3 < empty || pins->sdp3_3 > out_B) ||
+	    (pins->gpio_4 < empty || pins->gpio_4 > out_B)) {
+		dev_warn(&pf->pdev->dev,
+			 "The provided PTP configuration set contains meaningless values that may potentially pose a safety threat.\n");
+		return -EPERM;
+	}
+
+	if (pins->sdp3_2 == empty)
 		pins->sdp3_2 = pf->ptp_pins->sdp3_2;
-	if (pins->sdp3_3 == invalid)
+	if (pins->sdp3_3 == empty)
 		pins->sdp3_3 = pf->ptp_pins->sdp3_3;
-	if (pins->gpio_4 == invalid)
+	if (pins->gpio_4 == empty)
 		pins->gpio_4 = pf->ptp_pins->gpio_4;
 	while (i40e_ptp_pin_led_allowed_states[i].sdp3_2 != end) {
 		if (pins->sdp3_2 == i40e_ptp_pin_led_allowed_states[i].sdp3_2 &&
-- 
2.38.1

Re: [PATCH net 1/1] i40e: fix PTP pins verification

[Leon Romanovsky]

On Tue, Apr 25, 2023 at 10:04:06AM -0700, Tony Nguyen wrote:
> From: Andrii Staikov <andrii.staikov@intel.com>
> 
> Fix PTP pins verification not to contain tainted arguments. As a new PTP
> pins configuration is provided by a user, it may contain tainted
> arguments that are out of bounds for the list of possible values that can
> lead to a potential security threat. Change pin's state name from 'invalid'
> to 'empty' for more clarification.

And why isn't this handled in upper layer which responsible to get
user input?

Thanks

Re: [PATCH net 1/1] i40e: fix PTP pins verification

[Richard Cochran]

On Wed, Apr 26, 2023 at 10:18:12AM +0300, Leon Romanovsky wrote:
> On Tue, Apr 25, 2023 at 10:04:06AM -0700, Tony Nguyen wrote:
> > From: Andrii Staikov <andrii.staikov@intel.com>
> > 
> > Fix PTP pins verification not to contain tainted arguments. As a new PTP
> > pins configuration is provided by a user, it may contain tainted
> > arguments that are out of bounds for the list of possible values that can
> > lead to a potential security threat. Change pin's state name from 'invalid'
> > to 'empty' for more clarification.
> 
> And why isn't this handled in upper layer which responsible to get
> user input?

It is.

long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
{
	...

	switch (cmd) {

	case PTP_PIN_SETFUNC:
	case PTP_PIN_SETFUNC2:
		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
			err = -EFAULT;
			break;
		}
		...

		pin_index = pd.index;
		if (pin_index >= ops->n_pins) {
			err = -EINVAL;
			break;
		}

		...
	}
	...
}

Re: [PATCH net 1/1] i40e: fix PTP pins verification

[Paolo Abeni]

On Wed, 2023-04-26 at 06:52 -0700, Richard Cochran wrote:
> On Wed, Apr 26, 2023 at 10:18:12AM +0300, Leon Romanovsky wrote:
> > On Tue, Apr 25, 2023 at 10:04:06AM -0700, Tony Nguyen wrote:
> > > From: Andrii Staikov <andrii.staikov@intel.com>
> > > 
> > > Fix PTP pins verification not to contain tainted arguments. As a new PTP
> > > pins configuration is provided by a user, it may contain tainted
> > > arguments that are out of bounds for the list of possible values that can
> > > lead to a potential security threat. Change pin's state name from 'invalid'
> > > to 'empty' for more clarification.
> > 
> > And why isn't this handled in upper layer which responsible to get
> > user input?
> 
> It is.
> 
> long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> {
> 	...
> 
> 	switch (cmd) {
> 
> 	case PTP_PIN_SETFUNC:
> 	case PTP_PIN_SETFUNC2:
> 		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
> 			err = -EFAULT;
> 			break;
> 		}
> 		...
> 
> 		pin_index = pd.index;
> 		if (pin_index >= ops->n_pins) {
> 			err = -EINVAL;
> 			break;
> 		}
> 
> 		...
> 	}
> 	...
> }

Given the above, I don't see why/how this patch is necessary? @Tony,
@Andrii: could you please give a better/longer description of the issue
addressed here?

Thanks!

Paolo

Re: [PATCH net 1/1] i40e: fix PTP pins verification

[Tony Nguyen]

On 4/27/2023 3:24 AM, Paolo Abeni wrote:
> On Wed, 2023-04-26 at 06:52 -0700, Richard Cochran wrote:
>> On Wed, Apr 26, 2023 at 10:18:12AM +0300, Leon Romanovsky wrote:
>>> On Tue, Apr 25, 2023 at 10:04:06AM -0700, Tony Nguyen wrote:
>>>> From: Andrii Staikov <andrii.staikov@intel.com>
>>>>
>>>> Fix PTP pins verification not to contain tainted arguments. As a new PTP
>>>> pins configuration is provided by a user, it may contain tainted
>>>> arguments that are out of bounds for the list of possible values that can
>>>> lead to a potential security threat. Change pin's state name from 'invalid'
>>>> to 'empty' for more clarification.
>>>
>>> And why isn't this handled in upper layer which responsible to get
>>> user input?
>>
>> It is.
>>
>> long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
>> {
>> 	...
>>
>> 	switch (cmd) {
>>
>> 	case PTP_PIN_SETFUNC:
>> 	case PTP_PIN_SETFUNC2:
>> 		if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
>> 			err = -EFAULT;
>> 			break;
>> 		}
>> 		...
>>
>> 		pin_index = pd.index;
>> 		if (pin_index >= ops->n_pins) {
>> 			err = -EINVAL;
>> 			break;
>> 		}
>>
>> 		...
>> 	}
>> 	...
>> }
> 
> Given the above, I don't see why/how this patch is necessary? @Tony,
> @Andrii: could you please give a better/longer description of the issue
> addressed here?

I'm not sure about the issue details; Andrii please chime in.

Thanks,
Tony

RE: [PATCH net 1/1] i40e: fix PTP pins verification

[Staikov, Andrii]

Hello!

> > On Wed, Apr 26, 2023 at 10:18:12AM +0300, Leon Romanovsky wrote:
> > > On Tue, Apr 25, 2023 at 10:04:06AM -0700, Tony Nguyen wrote:
> > > > From: Andrii Staikov andrii.staikov@intel.com
> > > > 
> > > > Fix PTP pins verification not to contain tainted arguments. As a new PTP
> > > > pins configuration is provided by a user, it may contain tainted
> > > > arguments that are out of bounds for the list of possible values that can
> > > > lead to a potential security threat. Change pin's state name from 'invalid'
> > > > to 'empty' for more clarification.
> > > 
> > > And why isn't this handled in upper layer which responsible to get
> > > user input?
> > 
> > It is.
> > 
> > long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
> > {
> >          ...
> > 
> >          switch (cmd) {
> > 
> >          case PTP_PIN_SETFUNC:
> >          case PTP_PIN_SETFUNC2:
> >                          if (copy_from_user(&pd, (void __user *)arg, sizeof(pd))) {
> >                                          err = -EFAULT;
> >                                          break;
> >                          }
> >                          ...
> > 
> >                          pin_index = pd.index;
> >                          if (pin_index >= ops->n_pins) {
> >                                          err = -EINVAL;
> >                                          break;
> >                          }
> > 
> >                          ...
> >          }
> >          ...
> > }

Actually, the provided code snippet 
if (pin_index >= ops->n_pins) {
                err = -EINVAL;
                break;
}
shows that the check happens only to the number of pins, but not their value.

The list of the possible values is defined in the i40e_ptp_gpio_pin_state enum:
enum i40e_ptp_gpio_pin_state {
                end = -2,
                invalid,
                off,
                in_A,
                in_B,
                out_A,
                out_B,
};

Despite having the 'invalid' value (which I also consider not the best naming as in fact it means an empty value), all the values bellow the 'invalid' and above the 'out_B' are invalid, and since they are provided by a user, nothing guarantees them to be in range of valid values. I don't see such check and suggest adding it here.
Besides that I suggest changing naming of 'invalid' state to 'empty' as this is just much logical to me as in fact this is what it is. 

> 
> Given the above, I don't see why/how this patch is necessary? @Tony,
> @Andrii: could you please give a better/longer description of the issue
> addressed here?
> 
> Thanks!
> 
> Paolo

Regards,
Staikov Andrii

Re: [PATCH net 1/1] i40e: fix PTP pins verification

[Richard Cochran]

On Thu, May 04, 2023 at 11:12:07AM +0000, Staikov, Andrii wrote:

> Actually, the provided code snippet 
> if (pin_index >= ops->n_pins) {
>                 err = -EINVAL;
>                 break;
> }
> shows that the check happens only to the number of pins, but not their value.

The pin function from the user is checked in ptp_set_pinfunc();

> The list of the possible values is defined in the i40e_ptp_gpio_pin_state enum:
> enum i40e_ptp_gpio_pin_state {
>                 end = -2,
>                 invalid,
>                 off,
>                 in_A,
>                 in_B,
>                 out_A,
>                 out_B,
> };

Those are not passed by the user.

Thanks,
Richard