[Intel-wired-lan] [PATCH net v1] i40e: fix PTP pins verification

[Staikov, Andrii]

Hello Paul!

Thank you for reviewing my patch.

>> Please use the full line width of 75 characters per line.
Okay, will do.

>> Also, how did you test this?
As it was written in the commit message, the values of a new pin configuration are provided by a user. These values are defined in i40e_ptp_gpio_pin_state enum, however nothing prevents a user to provide malicious values that are out of the bounds for the given values of the enum. That can be exploited. My patch adds a check for the provided values to be inside the given range.

>> Would it be better to tell the user the wrong argument?
I am not sure this is correct to tell a user what of the provided values are incorrect due the reasons above.

Regards,
Staikov Andrii

-----Original Message-----
From: Paul Menzel <pmenzel at molgen.mpg.de> 
Sent: Thursday, March 30, 2023 9:29 AM
To: Staikov, Andrii <andrii.staikov at intel.com>
Cc: intel-wired-lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH net v1] i40e: fix PTP pins verification

Dear Andrii,


Am 30.03.23 um 09:17 schrieb Andrii Staikov:
> 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.

Please use the full line width of 75 characters per line.

Also, how did you test this?

> Fixes: 1050713026a0 ("i40e: add support for PTP external 
> synchronization clock")
> Signed-off-by: Andrii Staikov <andrii.staikov at 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");

Would it be better to tell the user the wrong argument?

> +		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 &&


Kind regards,

Paul