Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1360897 > unrolled thread

[PATCH] mpt3sas: Don't overreach ioc->reply_post[] during initialization

Started byCalvin Owens <calvinowens@fb.com>
First post2016-03-18 20:50 +0100
Last post2016-03-23 21:40 +0100
Articles 7 — 5 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during initialization Calvin Owens <calvinowens@fb.com> - 2016-03-18 20:50 +0100
    Re: [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during initialization Johannes Thumshirn <jthumshirn@suse.de> - 2016-03-21 09:30 +0100
    Re: [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during initialization "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-03-22 01:40 +0100
      RE: [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during initialization Chaitra Basappa <chaitra.basappa@broadcom.com> - 2016-03-22 11:10 +0100
      RE: [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during initialization Chaitra Basappa <chaitra.basappa@broadcom.com> - 2016-03-23 10:20 +0100
    Re: [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during  initialization Jens Axboe <axboe@fb.com> - 2016-03-22 17:50 +0100
    Re: [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during initialization "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-03-23 21:40 +0100

#1360897 — [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during initialization

FromCalvin Owens <calvinowens@fb.com>
Date2016-03-18 20:50 +0100
Subject[PATCH] mpt3sas: Don't overreach ioc->reply_post[] during initialization
Message-ID<re8zg-6xu-23@gated-at.bofh.it>
In _base_make_ioc_operational(), we walk ioc->reply_queue_list and pull
a pointer out of successive elements of ioc->reply_post[] for each entry
in that list if RDPQ is enabled.

Since the code pulls the pointer for the next iteration at the bottom of
the loop, it triggers the a KASAN dump on the final iteration:

    BUG: KASAN: slab-out-of-bounds in _base_make_ioc_operational+0x47b7/0x47e0 [mpt3sas] at addr ffff880754816ab0
    Read of size 8 by task modprobe/305
    <snip>
    Call Trace:
     [<ffffffff81dfc591>] dump_stack+0x4d/0x6c
     [<ffffffff814c9689>] print_trailer+0xf9/0x150
     [<ffffffff814ceda4>] object_err+0x34/0x40
     [<ffffffff814d1231>] kasan_report_error+0x221/0x530
     [<ffffffff814d1673>] __asan_report_load8_noabort+0x43/0x50
     [<ffffffffa0043637>] _base_make_ioc_operational+0x47b7/0x47e0 [mpt3sas]
     [<ffffffffa0049a51>] mpt3sas_base_attach+0x1991/0x2120 [mpt3sas]
     [<ffffffffa0053c93>] _scsih_probe+0xeb3/0x16b0 [mpt3sas]
     [<ffffffff81ebd047>] local_pci_probe+0xc7/0x170
     [<ffffffff81ebf2cf>] pci_device_probe+0x20f/0x290
     [<ffffffff820d50cd>] really_probe+0x17d/0x600
     [<ffffffff820d56a3>] __driver_attach+0x153/0x190
     [<ffffffff820cffac>] bus_for_each_dev+0x11c/0x1a0
     [<ffffffff820d421d>] driver_attach+0x3d/0x50
     [<ffffffff820d378a>] bus_add_driver+0x44a/0x5f0
     [<ffffffff820d666c>] driver_register+0x18c/0x3b0
     [<ffffffff81ebcb76>] __pci_register_driver+0x156/0x200
     [<ffffffffa00c8135>] _mpt3sas_init+0x135/0x1000 [mpt3sas]
     [<ffffffff81000423>] do_one_initcall+0x113/0x2b0
     [<ffffffff813caa5a>] do_init_module+0x1d0/0x4d8
     [<ffffffff81273909>] load_module+0x6729/0x8dc0
     [<ffffffff81276123>] SYSC_init_module+0x183/0x1a0
     [<ffffffff8127625e>] SyS_init_module+0xe/0x10
     [<ffffffff828fe7d7>] entry_SYSCALL_64_fastpath+0x12/0x6a

Fix this by pulling the value at the beginning of the loop.

Signed-off-by: Calvin Owens <calvinowens@fb.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index e4db5fb..8c44b9c 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -5030,7 +5030,7 @@ _base_make_ioc_ready(struct MPT3SAS_ADAPTER *ioc, int sleep_flag,
 static int
 _base_make_ioc_operational(struct MPT3SAS_ADAPTER *ioc, int sleep_flag)
 {
-	int r, i;
+	int r, i, index;
 	unsigned long	flags;
 	u32 reply_address;
 	u16 smid;
@@ -5039,8 +5039,7 @@ _base_make_ioc_operational(struct MPT3SAS_ADAPTER *ioc, int sleep_flag)
 	struct _event_ack_list *delayed_event_ack, *delayed_event_ack_next;
 	u8 hide_flag;
 	struct adapter_reply_queue *reply_q;
-	long reply_post_free;
-	u32 reply_post_free_sz, index = 0;
+	Mpi2ReplyDescriptorsUnion_t *reply_post_free_contig;
 
 	dinitprintk(ioc, pr_info(MPT3SAS_FMT "%s\n", ioc->name,
 	    __func__));
@@ -5124,27 +5123,27 @@ _base_make_ioc_operational(struct MPT3SAS_ADAPTER *ioc, int sleep_flag)
 		_base_assign_reply_queues(ioc);
 
 	/* initialize Reply Post Free Queue */
-	reply_post_free_sz = ioc->reply_post_queue_depth *
-	    sizeof(Mpi2DefaultReplyDescriptor_t);
-	reply_post_free = (long)ioc->reply_post[index].reply_post_free;
+	index = 0;
+	reply_post_free_contig = ioc->reply_post[0].reply_post_free;
 	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
+		/*
+		 * If RDPQ is enabled, switch to the next allocation.
+		 * Otherwise advance within the contiguous region.
+		 */
+		if (ioc->rdpq_array_enable) {
+			reply_q->reply_post_free =
+				ioc->reply_post[index++].reply_post_free;
+		} else {
+			reply_q->reply_post_free = reply_post_free_contig;
+			reply_post_free_contig += ioc->reply_post_queue_depth;
+		}
+
 		reply_q->reply_post_host_index = 0;
-		reply_q->reply_post_free = (Mpi2ReplyDescriptorsUnion_t *)
-		    reply_post_free;
 		for (i = 0; i < ioc->reply_post_queue_depth; i++)
 			reply_q->reply_post_free[i].Words =
 			    cpu_to_le64(ULLONG_MAX);
 		if (!_base_is_controller_msix_enabled(ioc))
 			goto skip_init_reply_post_free_queue;
-		/*
-		 * If RDPQ is enabled, switch to the next allocation.
-		 * Otherwise advance within the contiguous region.
-		 */
-		if (ioc->rdpq_array_enable)
-			reply_post_free = (long)
-			    ioc->reply_post[++index].reply_post_free;
-		else
-			reply_post_free += reply_post_free_sz;
 	}
  skip_init_reply_post_free_queue:
 
-- 
2.8.0.rc2

[toc] | [next] | [standalone]

#1361601

FromJohannes Thumshirn <jthumshirn@suse.de>
Date2016-03-21 09:30 +0100
Message-ID<rf3nQ-2Wt-23@gated-at.bofh.it>
In reply to#1360897
On Freitag, 18. März 2016 12:45:42 CET Calvin Owens wrote:
> In _base_make_ioc_operational(), we walk ioc->reply_queue_list and pull
> a pointer out of successive elements of ioc->reply_post[] for each entry
> in that list if RDPQ is enabled.
> 
> Since the code pulls the pointer for the next iteration at the bottom of
> the loop, it triggers the a KASAN dump on the final iteration:
> 
>     BUG: KASAN: slab-out-of-bounds in
> _base_make_ioc_operational+0x47b7/0x47e0 [mpt3sas] at addr ffff880754816ab0
> Read of size 8 by task modprobe/305
>     <snip>
>     Call Trace:
>      [<ffffffff81dfc591>] dump_stack+0x4d/0x6c
>      [<ffffffff814c9689>] print_trailer+0xf9/0x150
>      [<ffffffff814ceda4>] object_err+0x34/0x40
>      [<ffffffff814d1231>] kasan_report_error+0x221/0x530
>      [<ffffffff814d1673>] __asan_report_load8_noabort+0x43/0x50
>      [<ffffffffa0043637>] _base_make_ioc_operational+0x47b7/0x47e0 [mpt3sas]
> [<ffffffffa0049a51>] mpt3sas_base_attach+0x1991/0x2120 [mpt3sas]
> [<ffffffffa0053c93>] _scsih_probe+0xeb3/0x16b0 [mpt3sas]
>      [<ffffffff81ebd047>] local_pci_probe+0xc7/0x170
>      [<ffffffff81ebf2cf>] pci_device_probe+0x20f/0x290
>      [<ffffffff820d50cd>] really_probe+0x17d/0x600
>      [<ffffffff820d56a3>] __driver_attach+0x153/0x190
>      [<ffffffff820cffac>] bus_for_each_dev+0x11c/0x1a0
>      [<ffffffff820d421d>] driver_attach+0x3d/0x50
>      [<ffffffff820d378a>] bus_add_driver+0x44a/0x5f0
>      [<ffffffff820d666c>] driver_register+0x18c/0x3b0
>      [<ffffffff81ebcb76>] __pci_register_driver+0x156/0x200
>      [<ffffffffa00c8135>] _mpt3sas_init+0x135/0x1000 [mpt3sas]
>      [<ffffffff81000423>] do_one_initcall+0x113/0x2b0
>      [<ffffffff813caa5a>] do_init_module+0x1d0/0x4d8
>      [<ffffffff81273909>] load_module+0x6729/0x8dc0
>      [<ffffffff81276123>] SYSC_init_module+0x183/0x1a0
>      [<ffffffff8127625e>] SyS_init_module+0xe/0x10
>      [<ffffffff828fe7d7>] entry_SYSCALL_64_fastpath+0x12/0x6a
> 
> Fix this by pulling the value at the beginning of the loop.
> 
> Signed-off-by: Calvin Owens <calvinowens@fb.com>

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [next] | [standalone]

#1362241

From"Martin K. Petersen" <martin.petersen@oracle.com>
Date2016-03-22 01:40 +0100
Message-ID<rfiwy-4XQ-1@gated-at.bofh.it>
In reply to#1360897
>>>>> "Calvin" == Calvin Owens <calvinowens@fb.com> writes:

Calvin> In _base_make_ioc_operational(), we walk ioc->reply_queue_list
Calvin> and pull a pointer out of successive elements of
Calvin> ioc->reply_post[] for each entry in that list if RDPQ is
Calvin> enabled.

Calvin> Since the code pulls the pointer for the next iteration at the
Calvin> bottom of the loop, it triggers the a KASAN dump on the final
Calvin> iteration:

Broadcom folks, please review.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1362450

FromChaitra Basappa <chaitra.basappa@broadcom.com>
Date2016-03-22 11:10 +0100
Message-ID<rfrq9-2RD-3@gated-at.bofh.it>
In reply to#1362241
Martin,
 This patch is being reviewed , we shall get back with reviews by
tomorrow.

Thanks,
 Chaitra

-----Original Message-----
From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
Sent: Tuesday, March 22, 2016 6:00 AM
To: Calvin Owens
Cc: Sathya Prakash; Chaitra P B; Suganath Prabu Subramani; James E.J.
Bottomley; Martin K. Petersen; MPT-FusionLinux.pdl@broadcom.com;
linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org;
kernel-team@fb.com; Sreekanth Reddy
Subject: Re: [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during
initialization

>>>>> "Calvin" == Calvin Owens <calvinowens@fb.com> writes:

Calvin> In _base_make_ioc_operational(), we walk ioc->reply_queue_list
Calvin> and pull a pointer out of successive elements of
Calvin> ioc->reply_post[] for each entry in that list if RDPQ is
Calvin> enabled.

Calvin> Since the code pulls the pointer for the next iteration at the
Calvin> bottom of the loop, it triggers the a KASAN dump on the final
Calvin> iteration:

Broadcom folks, please review.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1363268

FromChaitra Basappa <chaitra.basappa@broadcom.com>
Date2016-03-23 10:20 +0100
Message-ID<rfN7k-1fh-5@gated-at.bofh.it>
In reply to#1362241
Hi,
 Please consider this patch as Ack-by: Chaitra P B
<chaitra.basappa@broadcom.com>

Thanks,
 Chaitra

-----Original Message-----
From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
Sent: Tuesday, March 22, 2016 6:00 AM
To: Calvin Owens
Cc: Sathya Prakash; Chaitra P B; Suganath Prabu Subramani; James E.J.
Bottomley; Martin K. Petersen; MPT-FusionLinux.pdl@broadcom.com;
linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org;
kernel-team@fb.com; Sreekanth Reddy
Subject: Re: [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during
initialization

>>>>> "Calvin" == Calvin Owens <calvinowens@fb.com> writes:

Calvin> In _base_make_ioc_operational(), we walk ioc->reply_queue_list
Calvin> and pull a pointer out of successive elements of
Calvin> ioc->reply_post[] for each entry in that list if RDPQ is
Calvin> enabled.

Calvin> Since the code pulls the pointer for the next iteration at the
Calvin> bottom of the loop, it triggers the a KASAN dump on the final
Calvin> iteration:

Broadcom folks, please review.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1362845 — Re: [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during initialization

FromJens Axboe <axboe@fb.com>
Date2016-03-22 17:50 +0100
SubjectRe: [PATCH] mpt3sas: Don't overreach ioc->reply_post[] during initialization
Message-ID<rfxFg-7gW-9@gated-at.bofh.it>
In reply to#1360897
On 03/18/2016 01:45 PM, Calvin Owens wrote:
> In _base_make_ioc_operational(), we walk ioc->reply_queue_list and pull
> a pointer out of successive elements of ioc->reply_post[] for each entry
> in that list if RDPQ is enabled.
>
> Since the code pulls the pointer for the next iteration at the bottom of
> the loop, it triggers the a KASAN dump on the final iteration:
>
>      BUG: KASAN: slab-out-of-bounds in _base_make_ioc_operational+0x47b7/0x47e0 [mpt3sas] at addr ffff880754816ab0
>      Read of size 8 by task modprobe/305
>      <snip>
>      Call Trace:
>       [<ffffffff81dfc591>] dump_stack+0x4d/0x6c
>       [<ffffffff814c9689>] print_trailer+0xf9/0x150
>       [<ffffffff814ceda4>] object_err+0x34/0x40
>       [<ffffffff814d1231>] kasan_report_error+0x221/0x530
>       [<ffffffff814d1673>] __asan_report_load8_noabort+0x43/0x50
>       [<ffffffffa0043637>] _base_make_ioc_operational+0x47b7/0x47e0 [mpt3sas]
>       [<ffffffffa0049a51>] mpt3sas_base_attach+0x1991/0x2120 [mpt3sas]
>       [<ffffffffa0053c93>] _scsih_probe+0xeb3/0x16b0 [mpt3sas]
>       [<ffffffff81ebd047>] local_pci_probe+0xc7/0x170
>       [<ffffffff81ebf2cf>] pci_device_probe+0x20f/0x290
>       [<ffffffff820d50cd>] really_probe+0x17d/0x600
>       [<ffffffff820d56a3>] __driver_attach+0x153/0x190
>       [<ffffffff820cffac>] bus_for_each_dev+0x11c/0x1a0
>       [<ffffffff820d421d>] driver_attach+0x3d/0x50
>       [<ffffffff820d378a>] bus_add_driver+0x44a/0x5f0
>       [<ffffffff820d666c>] driver_register+0x18c/0x3b0
>       [<ffffffff81ebcb76>] __pci_register_driver+0x156/0x200
>       [<ffffffffa00c8135>] _mpt3sas_init+0x135/0x1000 [mpt3sas]
>       [<ffffffff81000423>] do_one_initcall+0x113/0x2b0
>       [<ffffffff813caa5a>] do_init_module+0x1d0/0x4d8
>       [<ffffffff81273909>] load_module+0x6729/0x8dc0
>       [<ffffffff81276123>] SYSC_init_module+0x183/0x1a0
>       [<ffffffff8127625e>] SyS_init_module+0xe/0x10
>       [<ffffffff828fe7d7>] entry_SYSCALL_64_fastpath+0x12/0x6a
>
> Fix this by pulling the value at the beginning of the loop.
>
> Signed-off-by: Calvin Owens <calvinowens@fb.com>

Looks good to me.

Reviewed-by: Jens Axboe <axboe@fb.com>

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1363672

From"Martin K. Petersen" <martin.petersen@oracle.com>
Date2016-03-23 21:40 +0100
Message-ID<rfXJo-n8-13@gated-at.bofh.it>
In reply to#1360897
>>>>> "Calvin" == Calvin Owens <calvinowens@fb.com> writes:

Calvin> In _base_make_ioc_operational(), we walk ioc->reply_queue_list
Calvin> and pull a pointer out of successive elements of
Calvin> ioc->reply_post[] for each entry in that list if RDPQ is
Calvin> enabled.

Calvin> Since the code pulls the pointer for the next iteration at the
Calvin> bottom of the loop, it triggers the a KASAN dump on the final
Calvin> iteration:

Applied to 4.6/scsi-fixes.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web