On Wed, May 03, 2017 at 10:19:44PM -0700, Stephen Hemminger wrote: >On Wed, 3 May 2017 14:44:35 +1000 >Gavin Shan <gws...@linux.vnet.ibm.com> wrote: > >> +static int ethtool_get_ncsi_channels(struct net_device *dev, >> + void __user *useraddr) > >Please don't use an opaque type for this. See how other ethtool >operations take a struct. >
After checking output from below command, all other ethtool operations uses "void __user *" or "char __user *". git grep static.*useraddr net/core/ethtool.c >> +{ >> + struct ethtool_ncsi_channels *enc; >> + short nr_channels; >Should be __u16 or unsigned not short. > Nope, It's for signed number. User expects to get number of available channels when negative number is passed in. When it's positive, it's going to get the channels' information. >> + ssize_t size = 0; >> + int ret; >> + >> + if (!dev->ethtool_ops->get_ncsi_channels) >> + return -EOPNOTSUPP; >> + >> + if (copy_from_user(&nr_channels, useraddr + sizeof(enc->cmd), >> + sizeof(nr_channels))) >> + return -EFAULT; >> + >> + size = sizeof(*enc); >> + if (nr_channels > 0) >> + size += nr_channels * sizeof(enc->id[0]); > >You have no upper bound on number of channels, and therefore an incorrectly >application could grab an excessive amount of kernel memory. > Yeah, I'll limit it to 256 in next respin. 256 is the maximal number of channels for one particular net device. Cheers, Gavin