| Message ID | 20221208011122.2343363-14-jesse.brandeburg@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | ethtool: clean up and fix | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Dec 07, 2022 at 05:11:22PM -0800, Jesse Brandeburg wrote: > The parse_reset function was open-coding string parsing routines and can > be converted to using standard routines. It was trying to be tricky and > parse partial strings to make things like "mgmt" and "mgmt-shared" > strings mostly use the same code. This is better done with strncmp(). > > This also fixes an array overflow bug where it's possible for the for > loop to access past the end of the input array, which is bad. > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > --- > ethtool.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/ethtool.c b/ethtool.c > index 4776afe89e23..d294b5f8d92a 100644 > --- a/ethtool.c > +++ b/ethtool.c > @@ -5280,17 +5280,21 @@ static int do_get_phy_tunable(struct cmd_context *ctx) > static __u32 parse_reset(char *val, __u32 bitset, char *arg, __u32 *data) > { > __u32 bitval = 0; > - int i; > + int vallen = strlen(val); > + int strret; > > /* Check for component match */ > - for (i = 0; val[i] != '\0'; i++) > - if (arg[i] != val[i]) > - return 0; > + strret = strncmp(arg, val, vallen); > + if (strret < 0) > + return 0; Shouldn't this be "if (strret)"? Unless I missed something, this would lead to e.g. "zzzz-shared" being matched for "mgmt-shared" (with usual locale) because you would get strret > 0 here and strcmp() == 0 below. > > - /* Check if component has -shared specified or not */ > - if (arg[i] == '\0') > + /* if perfect match to val > + * else > + * Check if component has -shared specified or not > + */ > + if (strret == 0) > bitval = bitset; And this should be "if (!arg[vallen])", I believe. Michal > - else if (!strcmp(arg+i, "-shared")) > + else if (!strcmp(arg + vallen, "-shared")) > bitval = bitset << ETH_RESET_SHARED_SHIFT; > > if (bitval) { > -- > 2.31.1 >
diff --git a/ethtool.c b/ethtool.c index 4776afe89e23..d294b5f8d92a 100644 --- a/ethtool.c +++ b/ethtool.c @@ -5280,17 +5280,21 @@ static int do_get_phy_tunable(struct cmd_context *ctx) static __u32 parse_reset(char *val, __u32 bitset, char *arg, __u32 *data) { __u32 bitval = 0; - int i; + int vallen = strlen(val); + int strret; /* Check for component match */ - for (i = 0; val[i] != '\0'; i++) - if (arg[i] != val[i]) - return 0; + strret = strncmp(arg, val, vallen); + if (strret < 0) + return 0; - /* Check if component has -shared specified or not */ - if (arg[i] == '\0') + /* if perfect match to val + * else + * Check if component has -shared specified or not + */ + if (strret == 0) bitval = bitset; - else if (!strcmp(arg+i, "-shared")) + else if (!strcmp(arg + vallen, "-shared")) bitval = bitset << ETH_RESET_SHARED_SHIFT; if (bitval) {
The parse_reset function was open-coding string parsing routines and can be converted to using standard routines. It was trying to be tricky and parse partial strings to make things like "mgmt" and "mgmt-shared" strings mostly use the same code. This is better done with strncmp(). This also fixes an array overflow bug where it's possible for the for loop to access past the end of the input array, which is bad. Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> --- ethtool.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)