[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <PH0PR11MB561175B08D0E299FD1A0C127856D9@PH0PR11MB5611.namprd11.prod.outlook.com>
Date: Thu, 4 May 2023 11:12:07 +0000
From: "Staikov, Andrii" <andrii.staikov@...el.com>
To: Paolo Abeni <pabeni@...hat.com>, Richard Cochran
<richardcochran@...il.com>, Leon Romanovsky <leon@...nel.org>
CC: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "davem@...emloft.net"
<davem@...emloft.net>, "kuba@...nel.org" <kuba@...nel.org>,
"edumazet@...gle.com" <edumazet@...gle.com>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>, "Mekala, SunithaX D" <sunithax.d.mekala@...el.com>
Subject: RE: [PATCH net 1/1] i40e: fix PTP pins verification
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@...el.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
Powered by blists - more mailing lists