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: <df263bfa-9610-419b-8b17-623f5fb54d26@intel.com> Date: Sat, 9 Dec 2023 16:09:40 -0800 From: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com> To: Jakub Kicinski <kuba@...nel.org> CC: Tony Nguyen <anthony.l.nguyen@...el.com>, <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 v5 2/5] ice: configure FW logging On 12/7/2023 6:19 PM, Jakub Kicinski wrote: > On Thu, 7 Dec 2023 16:28:27 -0800 Paul M Stillwell Jr wrote: >> On 12/6/2023 7:53 PM, Jakub Kicinski wrote: >>> On Tue, 5 Dec 2023 13:12:45 -0800 Tony Nguyen wrote: >>>> +/** >>>> + * ice_debugfs_parse_cmd_line - Parse the command line that was passed in >>>> + * @src: pointer to a buffer holding the command line >>>> + * @len: size of the buffer in bytes >>>> + * @argv: pointer to store the command line items >>>> + * @argc: pointer to store the number of command line items >>>> + */ >>>> +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); >>>> + 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; >>>> + } >>>> + >>>> + *argv = argv_split(GFP_KERNEL, cmd_buf, argc); >>>> + kfree(cmd_buf); >>>> + if (!*argv) >>>> + return -ENOMEM; >>> >>> I haven't spotted a single caller wanting this full argc/argv parsing. >>> Can we please not add this complexity until its really needed? >>> >> >> I can remove it, but I use it in all the _write functions. I use the >> argc to make sure I'm only getting one value to a write and I use >> argv[0] to deal with the value. >> >> Honestly I'm not sure how valuable it is to check for a single argument, >> but I'm fairly certain our validation team will file a bug if they pass >> more than one argument and something happens :) > > Just eyeballing the code - you'd still accept > > echo -e 'val1\0val2' > file > > right? :) Perhaps less like that validation would come up with that > but even the standard debugfs implementations don't seem to care too > much (example of bool file in netdevsim): > > # cat bpf_bind_accept > Y > # echo 'nbla' > bpf_bind_accept > # echo $? > 0 > # cat bpf_bind_accept > N > Good point, changed >> Examples of using argv are on lines 358 and 466 of ice_debugfs.c >> >> I'm open to changing it, just not sure to what >> >>>> +/** >>>> + * ice_debugfs_module_read - read from 'module' file >>>> + * @filp: the opened file >>>> + * @buffer: where to write the data for the user to read >>>> + * @count: the size of the user's buffer >>>> + * @ppos: file position offset >>>> + */ >>>> +static ssize_t ice_debugfs_module_read(struct file *filp, char __user *buffer, >>>> + size_t count, loff_t *ppos) >>>> +{ >>>> + struct dentry *dentry = filp->f_path.dentry; >>>> + struct ice_pf *pf = filp->private_data; >>>> + int status, module; >>>> + char *data = NULL; >>>> + >>>> + /* don't allow commands if the FW doesn't support it */ >>>> + if (!ice_fwlog_supported(&pf->hw)) >>>> + return -EOPNOTSUPP; >>>> + >>>> + module = ice_find_module_by_dentry(pf, dentry); >>>> + if (module < 0) { >>>> + dev_info(ice_pf_to_dev(pf), "unknown module\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + data = vzalloc(ICE_AQ_MAX_BUF_LEN); >>>> + if (!data) { >>>> + dev_warn(ice_pf_to_dev(pf), "Unable to allocate memory for FW configuration!\n"); >>>> + return -ENOMEM; >>> >>> Can we use seq_print() here? It should simplify the reading quite a bit, >>> not sure how well it works with files that can also be written, tho. >>> >> >> I'm probably missing something here, but how do we get more simple than >> snprintf? I have a function (ice_fwlog_print_module_cfg) that handles >> whether the user has passed a single module ID or they want data on all >> the modules, but it all boils down to snprintf. > > You need to vzalloc() and worry about it overflowing. > >> I could get rid of ice_fwlog_print_module_cfg() and replace it inline >> with the if/else code if that would be clearer, but I'm not sure >> seq_printf() is helpful because each file is a single quantum of >> information (with the exception of the file that represents all the >> modules). I created a special file to represent all the modules, but >> maybe it's more confusing and I should get rid of it and just make the >> users specify all of the modules in a script. >> >> Would that be easier? Then there is no if/else it's just a single snprintf. > > The value of seq_print() here is the fact it will handle the memory > allocation and copying to user space for you. Ignore the "seq" > in the name. Ah, I see your point now, thanks for the clarification. I've made this change to seq_printf() for printing the module info. This brings up the question of whether I should use seq_printf() for all the other _read fucntions. It feels like a lot of extra code to do it for the other _read functions because they output so little info and we control the output so it seems to be overkill to use seq_printf() for those. What do you think?
Powered by blists - more mailing lists