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
| ||
|
Message-ID: <835b8308-c2b1-097b-8b1c-e020647b5a33@intel.com> Date: Tue, 10 Oct 2023 16:26:15 -0700 From: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com> To: Jakub Kicinski <kuba@...nel.org>, Tony Nguyen <anthony.l.nguyen@...el.com> CC: <davem@...emloft.net>, <pabeni@...hat.com>, <edumazet@...gle.com>, <netdev@...r.kernel.org>, <jacob.e.keller@...el.com>, <vaishnavi.tipireddy@...el.com>, <horms@...nel.org>, <leon@...nel.org>, Pucha Himasekhar Reddy <himasekharx.reddy.pucha@...el.com> Subject: Re: [PATCH net-next v4 2/5] ice: configure FW logging On 10/6/2023 5:02 PM, Jakub Kicinski wrote: > On Thu, 5 Oct 2023 10:01:07 -0700 Tony Nguyen wrote: >> +static ssize_t ice_debugfs_parse_cmd_line(const char __user *src, size_t len, >> + char ***argv, int *argc) >> +{ >> + char *cmd_buf, *cmd_buf_tmp; >> + >> + cmd_buf = memdup_user(src, len + 1); > > memdup() with len + 1 is quite suspicious, the buffer has length > of len you shouldn't copy more than that > Yeah, you're right. I'll double check that... IIRC there was a reason I did it; for some reason I think it had to do with the CRLF from the user input, but that may have been an issue in a previous iteration. >> + if (IS_ERR(cmd_buf)) >> + return PTR_ERR(cmd_buf); >> + cmd_buf[len] = '\0'; >> + >> + /* the cmd_buf has a newline at the end of the command so >> + * remove it >> + */ >> + cmd_buf_tmp = strchr(cmd_buf, '\n'); >> + if (cmd_buf_tmp) { >> + *cmd_buf_tmp = '\0'; >> + len = (size_t)cmd_buf_tmp - (size_t)cmd_buf + 1; >> + } >> + >> + *argv = argv_split(GFP_KERNEL, cmd_buf, argc); >> + if (!*argv) >> + return -ENOMEM; >> + >> + kfree(cmd_buf); >> + return 0; > >> +static ssize_t >> +ice_debugfs_module_write(struct file *filp, const char __user *buf, >> + size_t count, loff_t *ppos) >> +{ >> + struct ice_pf *pf = filp->private_data; >> + struct device *dev = ice_pf_to_dev(pf); >> + ssize_t ret; >> + char **argv; >> + int argc; >> + >> + /* don't allow commands if the FW doesn't support it */ >> + if (!ice_fwlog_supported(&pf->hw)) >> + return -EOPNOTSUPP; >> + >> + /* don't allow partial writes */ >> + if (*ppos != 0) >> + return 0; >> + >> + ret = ice_debugfs_parse_cmd_line(buf, count, &argv, &argc); >> + if (ret) >> + goto err_copy_from_user; >> + >> + if (argc == 2) { >> + int module, log_level; >> + >> + module = sysfs_match_string(ice_fwlog_module_string, argv[0]); >> + if (module < 0) { >> + dev_info(dev, "unknown module '%s'\n", argv[0]); >> + ret = -EINVAL; >> + goto module_write_error; >> + } >> + >> + log_level = sysfs_match_string(ice_fwlog_level_string, argv[1]); >> + if (log_level < 0) { >> + dev_info(dev, "unknown log level '%s'\n", argv[1]); >> + ret = -EINVAL; >> + goto module_write_error; >> + } > > The parsing looks pretty over-engineered. > > You can group the entries into structs like this: > > struct something { > const char *name; > size_t sz; > enum whatever value; > }; > #define FILL_IN_STH(thing) \ > { .name = thing, sz = sizeof(thing) - 1, value = ICE_..##thing,} > > struct something[] = { > FILL_IN_STH(ALL), > FILL_IN_STH(MNG), > ... > }; > > but with nicer names > > Then just: > > for entry in array(..) { > if !strncmp(input, entry->name, entry->sz) { > str += entry->sz + 1 > found = entry; > break > } > } > I'm probably missing something here, but I don't know if this will do what I need or not. What I have is a user passing a module name and a log level and I'm trying to match those strings and create integer values from them so I can configure the FW log for that module. I'm not seeing how the above gets me there... I was trying to not use strncmp and instead use the built in kernel string matching functions so that's how I ended up with the code I have >> +static ssize_t ice_debugfs_resolution_read(struct file *filp, >> + char __user *buffer, size_t count, >> + loff_t *ppos) >> +{ >> + struct ice_pf *pf = filp->private_data; >> + struct ice_hw *hw = &pf->hw; >> + char buff[32] = {}; >> + int status; >> + >> + /* don't allow commands if the FW doesn't support it */ >> + if (!ice_fwlog_supported(&pf->hw)) >> + return -EOPNOTSUPP; >> + >> + snprintf(buff, sizeof(buff), "%d\n", >> + hw->fwlog_cfg.log_resolution); >> + >> + status = simple_read_from_buffer(buffer, count, ppos, buff, >> + strlen(buff)); >> + >> + return status; >> +} > >> +static ssize_t >> +ice_debugfs_resolution_write(struct file *filp, const char __user *buf, >> + size_t count, loff_t *ppos) >> +{ >> + struct ice_pf *pf = filp->private_data; >> + struct device *dev = ice_pf_to_dev(pf); >> + struct ice_hw *hw = &pf->hw; >> + ssize_t ret; >> + char **argv; >> + int argc; >> + >> + /* don't allow commands if the FW doesn't support it */ >> + if (!ice_fwlog_supported(hw)) >> + return -EOPNOTSUPP; >> + >> + /* don't allow partial writes */ >> + if (*ppos != 0) >> + return 0; >> + >> + ret = ice_debugfs_parse_cmd_line(buf, count, &argv, &argc); >> + if (ret) >> + goto err_copy_from_user; > > And for the simple params can you try to reuse existing debugfs > helpers? They can already read and write scalars, all you need > is to inject yourself on the write path to update the config > in the device. I would use existing debugfs helpers, but the ones that exist look like they are really set up for variables you want to read only. We need to do some validation on the input when the user is writing and I don't see how we could do that with the existing debugfs_create_* helpers. I've looked through the kernel code at uses of debugfs_create_* and couldn't find one that was setting up a writable file, but I could have missed it. If you can point me to some code that does what you want then I'm happy to try it, I just couldn't find anything.
Powered by blists - more mailing lists