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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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