RE: [PATCH v3 19/39] megaraid_sas: MR_TargetIdToLdGet u8 to u16 and avoid invalid raid-map access

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@xxxxxxxx]
> Sent: Friday, February 10, 2017 5:05 PM
> To: Shivasharan S; linux-scsi@xxxxxxxxxxxxxxx
> Cc: martin.petersen@xxxxxxxxxx; thenzl@xxxxxxxxxx;
> jejb@xxxxxxxxxxxxxxxxxx; kashyap.desai@xxxxxxxxxxxx;
> sumit.saxena@xxxxxxxxxxxx
> Subject: Re: [PATCH v3 19/39] megaraid_sas: MR_TargetIdToLdGet u8 to u16
> and avoid invalid raid-map access
>
> On 02/10/2017 09:59 AM, Shivasharan S wrote:
> > Change MR_TargetIdToLdGet return type from u8 to u16.
> >
> > ld id range check is added at two places in this patch -
> > @megasas_build_ldio_fusion and @megasas_build_ld_nonrw_fusion.
> > Previous driver code used different data type for lds TargetId
returned from
> MR_TargetIdToLdGet.
> > Prior to this change, above two functions was safeguarded due to
> > function always return u8 and maximum value of ld id returned was 255.
> >
> > In below check, fw_supported_vd_count as of today is 64 or 256 and
> > valid range  to support is either 0-63 or 0-255. Ideally want to
> > filter accessing raid map for ld ids which are not valid. With the u16
> > change, invalid ld id value is 0xFFFF and we will see kernel panic due
to random
> memory access in MR_LdRaidGet.
> > The changes will ensure we do not call MR_LdRaidGet if ld id is beyond
size of
> ldSpanMap array.
> >
> >                if (ld < instance->fw_supported_vd_count)
> >
> > From firmware perspective,ld id 0xFF is invalid and even though
> > current driver code forward such command, firmware fails with target
not
> available.
> >
> > ld target id issue occurs mainly whenever driver loops to populate
raid map
> (ea. MR_ValidateMapInfo).
> > These are the only two places where we may see out of range target ids
> > and wants to protect raid map access based on range provided by
Firmware
> API.
> >
> > Signed-off-by: Shivasharan S <shivasharan.srikanteshwara@xxxxxxxxxxxx>
> > Signed-off-by: Kashyap Desai <kashyap.desai@xxxxxxxxxxxx>
> > ---
> >
> > fix in v2 - updated description content.
> >
> >  drivers/scsi/megaraid/megaraid_sas.h        |  2 +-
> >  drivers/scsi/megaraid/megaraid_sas_fp.c     |  5 +++--
> >  drivers/scsi/megaraid/megaraid_sas_fusion.c | 25
> > ++++++++++++++-----------
> >  3 files changed, 18 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> > b/drivers/scsi/megaraid/megaraid_sas.h
> > index 0a20fff..efc01a3 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas.h
> > +++ b/drivers/scsi/megaraid/megaraid_sas.h
> > @@ -2448,7 +2448,7 @@ MR_BuildRaidContext(struct megasas_instance
> *instance,
> >  		    struct IO_REQUEST_INFO *io_info,
> >  		    struct RAID_CONTEXT *pRAID_Context,
> >  		    struct MR_DRV_RAID_MAP_ALL *map, u8 **raidLUN);
> > -u8 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL *map);
> > +u16 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL
> *map);
> >  struct MR_LD_RAID *MR_LdRaidGet(u32 ld, struct MR_DRV_RAID_MAP_ALL
> > *map);
> >  u16 MR_ArPdGet(u32 ar, u32 arm, struct MR_DRV_RAID_MAP_ALL *map);
> >  u16 MR_LdSpanArrayGet(u32 ld, u32 span, struct MR_DRV_RAID_MAP_ALL
> > *map); diff --git a/drivers/scsi/megaraid/megaraid_sas_fp.c
> > b/drivers/scsi/megaraid/megaraid_sas_fp.c
> > index a0b0e68..9d5d485 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fp.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fp.c
> > @@ -165,7 +165,7 @@ u16 MR_GetLDTgtId(u32 ld, struct
> MR_DRV_RAID_MAP_ALL *map)
> >  	return le16_to_cpu(map->raidMap.ldSpanMap[ld].ldRaid.targetId);
> >  }
> >
> > -u8 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL *map)
> > +u16 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL
> *map)
> >  {
> >  	return map->raidMap.ldTgtIdToLd[ldTgtId];
> >  }
> > @@ -1151,7 +1151,7 @@ MR_BuildRaidContext(struct megasas_instance
> > *instance,  {
> >  	struct fusion_context *fusion;
> >  	struct MR_LD_RAID  *raid;
> > -	u32         ld, stripSize, stripe_mask;
> > +	u32         stripSize, stripe_mask;
> >  	u64         endLba, endStrip, endRow, start_row, start_strip;
> >  	u64         regStart;
> >  	u32         regSize;
> > @@ -1163,6 +1163,7 @@ MR_BuildRaidContext(struct megasas_instance
> *instance,
> >  	u8	    retval = 0;
> >  	u8	    startlba_span = SPAN_INVALID;
> >  	u64 *pdBlock = &io_info->pdBlock;
> > +	u16	    ld;
> >
> >  	ldStartBlock = io_info->ldStartBlock;
> >  	numBlocks = io_info->numBlocks;
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index 9019b82..4aaf307 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -1121,7 +1121,8 @@ megasas_sync_map_info(struct megasas_instance
> *instance)
> >  	int i;
> >  	struct megasas_cmd *cmd;
> >  	struct megasas_dcmd_frame *dcmd;
> > -	u32 size_sync_info, num_lds;
> > +	u16 num_lds;
> > +	u32 size_sync_info;
> >  	struct fusion_context *fusion;
> >  	struct MR_LD_TARGET_SYNC *ci = NULL;
> >  	struct MR_DRV_RAID_MAP_ALL *map;
> > @@ -1870,7 +1871,7 @@ megasas_set_pd_lba(struct
> MPI2_RAID_SCSI_IO_REQUEST *io_request, u8 cdb_len,
> >  		   struct MR_DRV_RAID_MAP_ALL *local_map_ptr, u32 ref_tag)
> {
> >  	struct MR_LD_RAID *raid;
> > -	u32 ld;
> > +	u16 ld;
> >  	u64 start_blk = io_info->pdBlock;
> >  	u8 *cdb = io_request->CDB.CDB32;
> >  	u32 num_blocks = io_info->numBlocks; @@ -2303,10 +2304,11 @@
> > megasas_build_ldio_fusion(struct megasas_instance *instance,
> >
> >  	local_map_ptr = fusion->ld_drv_map[(instance->map_id & 1)];
> >  	ld = MR_TargetIdToLdGet(device_id, local_map_ptr);
> > -	raid = MR_LdRaidGet(ld, local_map_ptr);
> >
> > -	if ((MR_TargetIdToLdGet(device_id, local_map_ptr) >=
> > -		instance->fw_supported_vd_count) ||
(!fusion->fast_path_io)) {
> > +	if (ld < instance->fw_supported_vd_count)
> > +		raid = MR_LdRaidGet(ld, local_map_ptr);
> > +
> > +	if (!raid || (!fusion->fast_path_io)) {
> >  		io_request->RaidContext.raid_context.reg_lock_flags  = 0;
> >  		fp_possible = false;
> >  	} else {
> Is 'raid' correctly set to zero if the above condition is _not_ taken?

Hi Hannes,
'raid' is being initialized to NULL in megasas_build_ldio_fusion.
The initialization code is added in patch 2 of this series which does some
refactoring of code in this function.

-Shivasharan

>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke		               zSeries & Storage
> hare@xxxxxxxx			               +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton HRB 21284 (AG
> Nürnberg)




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux