Re: [RFC V2] drivers/virt/vSMP: new driver

[Dan Carpenter <dan.carpenter@oracle.com>]

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