On Sun, Apr 24, 2022 at 11:44:33AM +0000, Czerwacki, Eial wrote:
> +u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type)
> +{
> + u64 ret_val;
> +
> + switch (type) {
> + case VSMP_CTL_REG_SIZE_8BIT:
> + ret_val = readb(cfg_addr + reg);
> + break;
> +
> + case VSMP_CTL_REG_SIZE_16BIT:
> + ret_val = readw(cfg_addr + reg);
> + break;
> +
> + case VSMP_CTL_REG_SIZE_32BIT:
> + ret_val = readl(cfg_addr + reg);
> + break;
> +
> + case VSMP_CTL_REG_SIZE_64BIT:
> + ret_val = readq(cfg_addr + reg);
> + break;
> +
> + default:
> + printk(KERN_ERR "unsupported reg size type %d.\n", type);
> + ret_val = (u64) -EINVAL;
This error code gets truncated to a u16 or whatever so it doesn't work.
It's dead code anyway, right? Just return zero.
> + }
> +
> + dev_dbg(&vsmp_dev_obj->dev, "%s: read 0x%llx from reg 0x%llx of %d bits\n",
> + __func__, ret_val, reg, (type + 1) * 8);
> + return ret_val;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_read_reg_from_cfg);
[ snip ]
> +
> +/*
> + * deregister the vSMP sysfs object for user space interaction
> + */
> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr)
> +{
> + if (vsmp_sysfs_kobj && bin_attr)
> + sysfs_remove_bin_file(vsmp_sysfs_kobj, bin_attr);
> +}
> +EXPORT_SYMBOL_GPL(vsmp_deregister_sysfs_group);
> +
> +/*
> + * generic function to read from the bar
> + */
> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
> + char *buf, loff_t off, ssize_t count)
> +{
> + ssize_t ret_val = 0;
> +
> + if (op->buff_offset >= op->hwi_block_size) { // perform H/W op
> + vsmp_reset_op(op);
> +
> + ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff, op->hwi_block_size, false);
> + if (ret_val) {
> + printk(KERN_ERR "%s operation failed\n",
> + (op->action == FW_READ) ? "read" : "write");
This read vs write is weird. We're in a read function. Also it's
always read.
> + }
> + op->buff_offset = 0;
> + }
> +
> + if (ret_val)
> + return ret_val;
> +
> + return memory_read_from_buffer(buf, count, &op->buff_offset, op->buff, op->hwi_block_size);;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_generic_buff_read);
[ snip ]
> +
> +/*
> + * init the common module
> + */
> +static int __init vsmp_common_info_init(void)
> +{
> + int ret_val = -0, i;
^^
Negative zero?
> +
> + for (i = 0; (i < ARRAY_SIZE(cbs_arr) && !ret_val); i++) {
> + ret_val = cbs_arr[i].reg_cb();
> + if (ret_val) {
> + printk(KERN_ERR "failed to init sysfs entry %d (%d), exiting.\n",
> + i, ret_val);
> + }
> + }
> +
> + return ret_val;
> +}
[ snip ]
> +
> +/*
> + * this is the maximal possible length of the version which is a text string
> + * the real len is usually much smaller, thus the driver uses this once to read
> + * the version string and record it's actual len.
> + * from that point and on, the actual len will be used in each call.
> + */
> +#define VERSION_MAX_LEN (1 << 19)
> +
> +static struct fw_ops op;
Greg already complained about globals, but this one is particularly bad
because there is no locking so it will lead to corruption.
> +
> +static ssize_t version_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr,
> + char *buf, loff_t off, size_t count)
> +{
> + s64 reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> + ssize_t ret_val;
> +
> + if (reg_val < 0) {
> + printk(KERN_ERR "failed to value of reg 0x%x\n", VSMP_VERSION_REG);
> + return 0;
> + }
> +
> + ret_val = vsmp_generic_buff_read(&op, 0, reg_val, buf, off, count);
> + if (ret_val < 0) {
> + printk(KERN_ERR "failed to read version (%ld)\n", ret_val);
> + return 0;
> + }
> +
> + buf[ret_val++] = '\n';
You need to consider "count" before you print the newline. #corruption.
> +
> + return ret_val;
> +}
> +
> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM, version_read, NULL, VERSION_MAX_LEN);
> +
> +/*
> + * retreive str in order to determine the proper length.
> + * this is the best way to maintain backwards compatibility with all
> + * vSMP versions.
> + */
> +static ssize_t get_version_len(void)
> +{
> + ssize_t ret_val = 0;
This assignment is never used. It just disables static analysis for
uninitialized variables and invites bugs.
> + u64 reg_val;
> + char *version_str = kzalloc(VERSION_MAX_LEN, GFP_ATOMIC | __GFP_NOWARN);
> +
> + if (!version_str) {
> + printk(KERN_ERR "failed to allocate buffer\n");
> + return -ENOMEM;
> + }
> +
> + reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> + if (reg_val < 0) {
> + printk(KERN_ERR "failed to value of reg 0x%x\n", VSMP_VERSION_REG);
> + return 0;
> + }
> +
> + memset(version_str, 0, VERSION_MAX_LEN);
> + ret_val = vsmp_read_buff_from_bar(0, reg_val, version_str, VERSION_MAX_LEN, true);
> + if (ret_val) {
> + kfree(version_str);
> + printk(KERN_ERR "failed to read buffer from bar\n");
> + return ret_val;
> + }
> + kfree(version_str);
^^^^^^^^^^^^^^^^^^
> +
> + return strlen(version_str);
^^^^^^^^^^^^^^^^^^^
Use after free. Try running Smatch against your code to detect these
bugs.
https://staticthinking.wordpress.com/2022/04/25/how-to-run-smatch-on-your-code/
> +}
> +
> +/*
> + * register the version sysfs entry
> + */
> +int sysfs_register_version_cb(void)
> +{
> + ssize_t len = get_version_len();
> + int ret_val;
> +
> + if (len < 1) {
> + printk(KERN_ERR "failed to init vSMP version module\n");
> + return len;
What if len is zero? (Please return a proper error code).
> + }
> + version_raw_attr.size = len;
> +
> + if (vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {
> + printk(KERN_ERR "failed to init vSMP version op\n");
> + return -ENODEV;
> + }
> +
> + ret_val = vsmp_register_sysfs_group(&version_raw_attr);
> + if (ret_val) {
> + printk(KERN_ERR "failed to init vSMP version support\n");
> + vsmp_release_op(&op);
> + }
> +
> + return ret_val;
> +}
> +
[ snip ]
> +
> +/* module functions */
> +static ssize_t active_log_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr,
> + char *buf, loff_t off, size_t count)
> +{
> + ssize_t ret_val = 0;
> +
> + if (!op.in_progress) {
> + ret_val = mutex_lock_interruptible(&active_log_mutex);
> + if (ret_val) {
> + printk(KERN_ERR
> + "error in atemmpt to lock mutex (%lx)\n",
> + ret_val);
> + return 0;
> + }
> +
> + vsmp_write_reg16_to_cfg(VSMP_LOGS_CTRL_REG,
> + vsmp_read_reg16_from_cfg
> + (VSMP_LOGS_CTRL_REG) | LOGS_MASK);
> + op.in_progress = true;
> + vsmp_write_reg64_to_cfg(VSMP_LOGS_STATUS_REG, 0);
> + }
> +
> + if (vsmp_op_buffer_depleted(&op)) {
> + if (!(vsmp_read_reg16_from_cfg(VSMP_LOGS_CTRL_REG) & LOGS_MASK)) {
> + vsmp_reset_op(&op);
> + op.in_progress = 0;
> + mutex_unlock(&active_log_mutex);
> + return ret_val;
> + }
> + }
> +
> + ret_val = vsmp_generic_buff_read(&op, 1, active_log_start, buf, off, count);
> + if (ret_val < 0) {
> + printk(KERN_ERR "error reading from buffer\n");
> + mutex_unlock(&active_log_mutex);
> + return 0;
return ret_val;
> + }
> +
> + if (vsmp_op_buffer_depleted(&op)) {
> + vsmp_write_reg64_to_cfg(VSMP_LOGS_STATUS_REG,
> + ~0LL & ~(1LL << 0));
> + }
> +
> + return count;
This isn't correct necessarily. It should be something like
return min(count, ret_val);
> +}
regards,
dan carpenter
