Re: [ 10/21] target: Add link_magic for fabric allow_link destination target_items

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

On Fri, 2013-01-18 at 17:19 -0800, Greg Kroah-Hartman wrote:
> 3.4-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> commit 0ff8754981261a80f4b77db2536dfea92c2d4539 upstream.
> 
> This patch adds [dev,lun]_link_magic value assignment + checks within generic
> target_fabric_port_link() and target_fabric_mappedlun_link() code to ensure
> destination config_item *target_item sent from configfs_symlink() ->
> config_item_operations->allow_link() is the underlying se_device->dev_group
> and se_lun->lun_group that we expect to symlink.
[...] 
> --- a/drivers/target/target_core_fabric_configfs.c
> +++ b/drivers/target/target_core_fabric_configfs.c
> @@ -72,6 +72,12 @@ static int target_fabric_mappedlun_link(
>  	struct se_portal_group *se_tpg;
>  	struct config_item *nacl_ci, *tpg_ci, *tpg_ci_s, *wwn_ci, *wwn_ci_s;
>  	int ret = 0, lun_access;
> +
> +	if (lun->lun_link_magic != SE_LUN_LINK_MAGIC) {
> +		pr_err("Bad lun->lun_link_magic, not a valid lun_ci pointer:"
> +			" %p to struct lun: %p\n", lun_ci, lun);
> +		return -EFAULT;
> +	}
>  	/*
>  	 * Ensure that the source port exists
>  	 */
[...]

This is absolutely not a valid way of doing dynamic type checking.

The problem is that these structures:

> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -779,6 +779,8 @@ struct se_subsystem_dev {
>  };
>  
>  struct se_device {
> +#define SE_DEV_LINK_MAGIC			0xfeeddeef
> +	u32			dev_link_magic;
>  	/* RELATIVE TARGET PORT IDENTIFER Counter */
>  	u16			dev_rpti_counter;
>  	/* Used for SAM Task Attribute ordering */
> @@ -869,6 +871,8 @@ struct se_port_stat_grps {
>  };
>  
>  struct se_lun {
> +#define SE_LUN_LINK_MAGIC			0xffff7771
> +	u32			lun_link_magic;
>  	/* See transport_lun_status_table */
>  	enum transport_lun_status_table lun_status;
>  	u32			lun_access;

have struct config_group embedded at different places.  So you know
where the magic number is for the expected type but if you get an object
of the wrong type then you'll read some other area of memory that might
just happen to have the expected magic number anyway.

It seems like you need a superclass for all the structures exposed
through configfs, to hold the type information.  I would suggest that
you define something like:

struct se_config_group {
	struct config_group group;
	u32 magic;
};

and put that in place of the plain struct config_group.  You can then
safely check the magic number before looking at anything in the larger
structure.

Ben.

-- 
Ben Hutchings
All the simple programs have been written, and all the good names taken.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]