netdev - Re: [PATCH net-next v9 1/2] driver core: auxiliary bus: show auxiliary device IRQs

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2024070457-creatable-heroics-94cb@gregkh>
Date: Thu, 4 Jul 2024 12:41:07 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Shay Drory <shayd@...dia.com>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, davem@...emloft.net,
	kuba@...nel.org, edumazet@...gle.com, david.m.ertman@...el.com,
	rafael@...nel.org, ira.weiny@...el.com, linux-rdma@...r.kernel.org,
	leon@...nel.org, tariqt@...dia.com, Simon Horman <horms@...nel.org>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Parav Pandit <parav@...dia.com>
Subject: Re: [PATCH net-next v9 1/2] driver core: auxiliary bus: show
 auxiliary device IRQs

On Wed, Jul 03, 2024 at 10:38:57AM +0300, Shay Drory wrote:
> +/**
> + * auxiliary_device_sysfs_irq_add - add a sysfs entry for the given IRQ
> + * @auxdev: auxiliary bus device to add the sysfs entry.
> + * @irq: The associated interrupt number.
> + *
> + * This function should be called after auxiliary device have successfully
> + * received the irq.
> + * The driver is responsible to add a unique irq for the auxiliary device. The
> + * driver can invoke this function from multiple thread context safely for
> + * unique irqs of the auxiliary devices. The driver must not invoke this API
> + * multiple times if the irq is already added previously.
> + *
> + * Return: zero on success or an error code on failure.
> + */
> +int auxiliary_device_sysfs_irq_add(struct auxiliary_device *auxdev, int irq)
> +{
> +	struct auxiliary_irq_info *info __free(kfree) = NULL;
> +	struct device *dev = &auxdev->dev;
> +	char *name __free(kfree) = NULL;
> +	int ret;
> +
> +	ret = auxiliary_irq_dir_prepare(auxdev);
> +	if (ret)
> +		return ret;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	sysfs_attr_init(&info->sysfs_attr.attr);
> +	name = kasprintf(GFP_KERNEL, "%d", irq);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	ret = xa_insert(&auxdev->irqs, irq, info, GFP_KERNEL);
> +	if (ret)
> +		return ret;
> +
> +	info->sysfs_attr.attr.name = name;
> +	ret = sysfs_add_file_to_group(&dev->kobj, &info->sysfs_attr.attr,
> +				      auxiliary_irqs_group.name);
> +	if (ret)
> +		goto sysfs_add_err;
> +
> +	info->sysfs_attr.attr.name = no_free_ptr(name);

This assignment of a name AFTER it has been created is odd.  I think I
know why you are doing this, but please make it obvious and perhaps
solve it in a cleaner way.  Assigning this "deep" in a sysfs structure
is not ok.


> +	xa_store(&auxdev->irqs, irq, no_free_ptr(info), GFP_KERNEL);
> +	return 0;
> +
> +sysfs_add_err:
> +	xa_erase(&auxdev->irqs, irq);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_add);
> +
> +/**
> + * auxiliary_device_sysfs_irq_remove - remove a sysfs entry for the given IRQ
> + * @auxdev: auxiliary bus device to add the sysfs entry.
> + * @irq: the IRQ to remove.
> + *
> + * This function should be called to remove an IRQ sysfs entry.
> + * The driver must invoke this API when IRQ is released by the device.
> + */
> +void auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq)
> +{
> +	struct auxiliary_irq_info *info __free(kfree) =	xa_load(&auxdev->irqs, irq);

No verification that this is an actual entry before you dereferenced it?
Bold move...

greg k-h