lkml.org 
[lkml]   [2013]   [Jan]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [ 10/21] target: Add link_magic for fabric allow_link destination target_items
From
Date
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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2013-01-19 05:41    [W:0.189 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site