diff mbox series

[1/2] qla2xxx: Fix Crash due to NULL pointer access in qla2x00_sysfs_read_optrom()

Message ID 20190506205219.7842-2-hmadhani@marvell.com (mailing list archive)
State Rejected
Headers show
Series [1/2] qla2xxx: Fix Crash due to NULL pointer access in qla2x00_sysfs_read_optrom() | expand

Commit Message

Himanshu Madhani May 6, 2019, 8:52 p.m. UTC 
From: Quinn Tran <qutran@marvell.com>

commit c7702b8c2271 ("scsi: qla2xxx: Get mutex lock before checking
optrom_state") fixed crash while reading optrom data by adding mutex
locking. However, there can be still case where previous WRITE for
optrom buffer failed and then read_optrom() is called with NULL
optrom_buffer. This patch fixes access to read optrom data if the
buffers are NULL.

following stack trace is seen in the log file

[3130734.630350] BUG: unable to handle kernel NULL pointer dereference at           (null)
[3130734.630366] IP: [<ffffffff81287526>] memcpy+0x6/0x110
[3130734.630373] PGD 0
[3130734.630374] Oops: 0000 [#1] SMP
[3130734.630375] Modules linked in: iscsi_target_mod target_core_mod configfs ip_vs tcp_diag dccp_diag dccp inet_diag fuse nfsv3 nfs_acl nfsv4 auth_rpcgs>
[3130734.630401]  hwmon dm_mirror dm_region_hash dm_log dm_mod ipv6 autofs4 [last unloaded: emcpioc]
[3130734.630404] CPU 9
[3130734.630407] Pid: 14513, comm: udevadm Tainted: PF          O 3.8.13-118.10.2.el7uek.x86_64 #2 Oracle Corporation SUN SERVER X4-2       /ASSY,MB,X4-2>
[3130734.630409] RIP: 0010:[<ffffffff81287526>]  [<ffffffff81287526>] memcpy+0x6/0x110
[3130734.630411] RSP: 0018:ffff88036c7a3e48  EFLAGS: 00010206
[3130734.630411] RAX: ffff880106b0f000 RBX: 0000000000001000 RCX: 0000000000001000
[3130734.630412] RDX: 0000000000001000 RSI: 0000000000000000 RDI: ffff880106b0f000
[3130734.630413] RBP: ffff88036c7a3e68 R08: 0000000000001000 R09: 0000000000000007
[3130734.630414] R10: 0000000000000004 R11: 0000000000000005 R12: ffff88036c7a3e78
[3130734.630414] R13: 0000000000001000 R14: ffff881fc96945e0 R15: ffff881fc7e99ba8
[3130734.630415] FS:  00007f5e245948c0(0000) GS:ffff881fff320000(0000) knlGS:0000000000000000
[3130734.630416] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[3130734.630417] CR2: 0000000000000000 CR3: 0000000106a88000 CR4: 00000000001407e0
[3130734.630418] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[3130734.630418] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[3130734.630419] Process udevadm (pid: 14513, threadinfo ffff88036c7a2000, task ffff880ee5820500)
[3130734.630420] Stack:
[3130734.630421]  ffffffff811ad38c ffff881fc9694000 ffff880106b0f000 0000000000001000
[3130734.630424]  ffff88036c7a3ea0 ffffffffa02dea5c 0000000000000000 ffff881014fd9540
[3130734.630427]  ffff881010863dc0 ffff88036c7a3f50 0000000000001000 ffff88036c7a3f08
[3130734.630429] Call Trace:
[3130734.630435]  [<ffffffff811ad38c>] ? memory_read_from_buffer+0x3c/0x60
[3130734.630445]  [<ffffffffa02dea5c>] qla2x00_sysfs_read_optrom+0x9c/0xc0 [qla2xxx]
[3130734.630449]  [<ffffffff811fe96f>] read+0xdf/0x1f0
[3130734.630454]  [<ffffffff81187ff3>] vfs_read+0xa3/0x180
[3130734.630455]  [<ffffffff81188299>] sys_read+0x49/0xa0
[3130734.630461]  [<ffffffff810df3b6>] ? __audit_syscall_exit+0x1f6/0x2a0
[3130734.630467]  [<ffffffff815874f9>] system_call_fastpath+0x16/0x1b
[3130734.630467] Code: 43 58 48 2b 43 50 88 43 4e 5b 5d c3 66 0f 1f 84 00 00 00 00 00 e8 fb fb ff ff eb e2 90 90 90 90 90 90 90 90 90 48 89 f8 48 89 d1 <>
[3130734.630485] RIP  [<ffffffff81287526>] memcpy+0x6/0x110
[3130734.630486]  RSP <ffff88036c7a3e48>
[3130734.630487] CR2: 0000000000000000

Fixes: c7702b8c2271 ("scsi: qla2xxx: Get mutex lock before checking optrom_state")
Cc: stable@vger.kernel.org # 4.10
Signed-off-by: Quinn Tran <qutran@marvell.com>
Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
---
 drivers/scsi/qla2xxx/qla_attr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Ewan Milne May 8, 2019, 3:36 p.m. UTC | #1 
See below.

On Mon, 2019-05-06 at 13:52 -0700, Himanshu Madhani wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> commit c7702b8c2271 ("scsi: qla2xxx: Get mutex lock before checking
> optrom_state") fixed crash while reading optrom data by adding mutex
> locking. However, there can be still case where previous WRITE for
> optrom buffer failed and then read_optrom() is called with NULL
> optrom_buffer. This patch fixes access to read optrom data if the
> buffers are NULL.
> 
> following stack trace is seen in the log file
> 
> [3130734.630350] BUG: unable to handle kernel NULL pointer dereference at           (null)
> [3130734.630366] IP: [<ffffffff81287526>] memcpy+0x6/0x110
> [3130734.630373] PGD 0
> [3130734.630374] Oops: 0000 [#1] SMP
> [3130734.630375] Modules linked in: iscsi_target_mod target_core_mod configfs ip_vs tcp_diag dccp_diag dccp inet_diag fuse nfsv3 nfs_acl nfsv4 auth_rpcgs>
> [3130734.630401]  hwmon dm_mirror dm_region_hash dm_log dm_mod ipv6 autofs4 [last unloaded: emcpioc]
> [3130734.630404] CPU 9
> [3130734.630407] Pid: 14513, comm: udevadm Tainted: PF          O 3.8.13-118.10.2.el7uek.x86_64 #2 Oracle Corporation SUN SERVER X4-2       /ASSY,MB,X4-2>
> [3130734.630409] RIP: 0010:[<ffffffff81287526>]  [<ffffffff81287526>] memcpy+0x6/0x110
> [3130734.630411] RSP: 0018:ffff88036c7a3e48  EFLAGS: 00010206
> [3130734.630411] RAX: ffff880106b0f000 RBX: 0000000000001000 RCX: 0000000000001000
> [3130734.630412] RDX: 0000000000001000 RSI: 0000000000000000 RDI: ffff880106b0f000
> [3130734.630413] RBP: ffff88036c7a3e68 R08: 0000000000001000 R09: 0000000000000007
> [3130734.630414] R10: 0000000000000004 R11: 0000000000000005 R12: ffff88036c7a3e78
> [3130734.630414] R13: 0000000000001000 R14: ffff881fc96945e0 R15: ffff881fc7e99ba8
> [3130734.630415] FS:  00007f5e245948c0(0000) GS:ffff881fff320000(0000) knlGS:0000000000000000
> [3130734.630416] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [3130734.630417] CR2: 0000000000000000 CR3: 0000000106a88000 CR4: 00000000001407e0
> [3130734.630418] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [3130734.630418] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [3130734.630419] Process udevadm (pid: 14513, threadinfo ffff88036c7a2000, task ffff880ee5820500)
> [3130734.630420] Stack:
> [3130734.630421]  ffffffff811ad38c ffff881fc9694000 ffff880106b0f000 0000000000001000
> [3130734.630424]  ffff88036c7a3ea0 ffffffffa02dea5c 0000000000000000 ffff881014fd9540
> [3130734.630427]  ffff881010863dc0 ffff88036c7a3f50 0000000000001000 ffff88036c7a3f08
> [3130734.630429] Call Trace:
> [3130734.630435]  [<ffffffff811ad38c>] ? memory_read_from_buffer+0x3c/0x60
> [3130734.630445]  [<ffffffffa02dea5c>] qla2x00_sysfs_read_optrom+0x9c/0xc0 [qla2xxx]
> [3130734.630449]  [<ffffffff811fe96f>] read+0xdf/0x1f0
> [3130734.630454]  [<ffffffff81187ff3>] vfs_read+0xa3/0x180
> [3130734.630455]  [<ffffffff81188299>] sys_read+0x49/0xa0
> [3130734.630461]  [<ffffffff810df3b6>] ? __audit_syscall_exit+0x1f6/0x2a0
> [3130734.630467]  [<ffffffff815874f9>] system_call_fastpath+0x16/0x1b
> [3130734.630467] Code: 43 58 48 2b 43 50 88 43 4e 5b 5d c3 66 0f 1f 84 00 00 00 00 00 e8 fb fb ff ff eb e2 90 90 90 90 90 90 90 90 90 48 89 f8 48 89 d1 <>
> [3130734.630485] RIP  [<ffffffff81287526>] memcpy+0x6/0x110
> [3130734.630486]  RSP <ffff88036c7a3e48>
> [3130734.630487] CR2: 0000000000000000
> 
> Fixes: c7702b8c2271 ("scsi: qla2xxx: Get mutex lock before checking optrom_state")
> Cc: stable@vger.kernel.org # 4.10
> Signed-off-by: Quinn Tran <qutran@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_attr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> index 8d560c562e9c..0341f3340edb 100644
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c
> @@ -275,7 +275,8 @@ qla2x00_sysfs_read_optrom(struct file *filp, struct kobject *kobj,
>  
>  	mutex_lock(&ha->optrom_mutex);
>  
> -	if (ha->optrom_state != QLA_SREADING)
> +	if ((ha->optrom_state != QLA_SREADING) ||
> +	    !buf || !ha->optrom_buffer)
>  		goto out;
>  

In this case it appears as if no error is returned, since rval is always 0?
Presumably this is the intentional behavior if it's not in the SREADING state?

Why is the test for !buf necessary, since such a check is mostly not done here
except for qlini_mode_store().  Is that the actual null pointer encountered?
I don't see how ha->optrom_buffer would ever be NULL in the QLA_SREADING state.
How does an optrom write failure result in this?  

-Ewan

>  	rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
Himanshu Madhani May 13, 2019, 6:10 p.m. UTC | #2 
Hi Ewan, 


On 5/8/19, 8:36 AM, "Ewan D. Milne" <emilne@redhat.com> wrote:

    External Email
    
    ----------------------------------------------------------------------
    See below.
    
    On Mon, 2019-05-06 at 13:52 -0700, Himanshu Madhani wrote:
    > From: Quinn Tran <qutran@marvell.com>
    > 
    > commit c7702b8c2271 ("scsi: qla2xxx: Get mutex lock before checking
    > optrom_state") fixed crash while reading optrom data by adding mutex
    > locking. However, there can be still case where previous WRITE for
    > optrom buffer failed and then read_optrom() is called with NULL
    > optrom_buffer. This patch fixes access to read optrom data if the
    > buffers are NULL.
    > 
    > following stack trace is seen in the log file
    > 
    > [3130734.630350] BUG: unable to handle kernel NULL pointer dereference at           (null)
    > [3130734.630366] IP: [<ffffffff81287526>] memcpy+0x6/0x110
    > [3130734.630373] PGD 0
    > [3130734.630374] Oops: 0000 [#1] SMP
    > [3130734.630375] Modules linked in: iscsi_target_mod target_core_mod configfs ip_vs tcp_diag dccp_diag dccp inet_diag fuse nfsv3 nfs_acl nfsv4 auth_rpcgs>
    > [3130734.630401]  hwmon dm_mirror dm_region_hash dm_log dm_mod ipv6 autofs4 [last unloaded: emcpioc]
    > [3130734.630404] CPU 9
    > [3130734.630407] Pid: 14513, comm: udevadm Tainted: PF          O 3.8.13-118.10.2.el7uek.x86_64 #2 Oracle Corporation SUN SERVER X4-2       /ASSY,MB,X4-2>
    > [3130734.630409] RIP: 0010:[<ffffffff81287526>]  [<ffffffff81287526>] memcpy+0x6/0x110
    > [3130734.630411] RSP: 0018:ffff88036c7a3e48  EFLAGS: 00010206
    > [3130734.630411] RAX: ffff880106b0f000 RBX: 0000000000001000 RCX: 0000000000001000
    > [3130734.630412] RDX: 0000000000001000 RSI: 0000000000000000 RDI: ffff880106b0f000
    > [3130734.630413] RBP: ffff88036c7a3e68 R08: 0000000000001000 R09: 0000000000000007
    > [3130734.630414] R10: 0000000000000004 R11: 0000000000000005 R12: ffff88036c7a3e78
    > [3130734.630414] R13: 0000000000001000 R14: ffff881fc96945e0 R15: ffff881fc7e99ba8
    > [3130734.630415] FS:  00007f5e245948c0(0000) GS:ffff881fff320000(0000) knlGS:0000000000000000
    > [3130734.630416] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    > [3130734.630417] CR2: 0000000000000000 CR3: 0000000106a88000 CR4: 00000000001407e0
    > [3130734.630418] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
    > [3130734.630418] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
    > [3130734.630419] Process udevadm (pid: 14513, threadinfo ffff88036c7a2000, task ffff880ee5820500)
    > [3130734.630420] Stack:
    > [3130734.630421]  ffffffff811ad38c ffff881fc9694000 ffff880106b0f000 0000000000001000
    > [3130734.630424]  ffff88036c7a3ea0 ffffffffa02dea5c 0000000000000000 ffff881014fd9540
    > [3130734.630427]  ffff881010863dc0 ffff88036c7a3f50 0000000000001000 ffff88036c7a3f08
    > [3130734.630429] Call Trace:
    > [3130734.630435]  [<ffffffff811ad38c>] ? memory_read_from_buffer+0x3c/0x60
    > [3130734.630445]  [<ffffffffa02dea5c>] qla2x00_sysfs_read_optrom+0x9c/0xc0 [qla2xxx]
    > [3130734.630449]  [<ffffffff811fe96f>] read+0xdf/0x1f0
    > [3130734.630454]  [<ffffffff81187ff3>] vfs_read+0xa3/0x180
    > [3130734.630455]  [<ffffffff81188299>] sys_read+0x49/0xa0
    > [3130734.630461]  [<ffffffff810df3b6>] ? __audit_syscall_exit+0x1f6/0x2a0
    > [3130734.630467]  [<ffffffff815874f9>] system_call_fastpath+0x16/0x1b
    > [3130734.630467] Code: 43 58 48 2b 43 50 88 43 4e 5b 5d c3 66 0f 1f 84 00 00 00 00 00 e8 fb fb ff ff eb e2 90 90 90 90 90 90 90 90 90 48 89 f8 48 89 d1 <>
    > [3130734.630485] RIP  [<ffffffff81287526>] memcpy+0x6/0x110
    > [3130734.630486]  RSP <ffff88036c7a3e48>
    > [3130734.630487] CR2: 0000000000000000
    > 
    > Fixes: c7702b8c2271 ("scsi: qla2xxx: Get mutex lock before checking optrom_state")
    > Cc: stable@vger.kernel.org # 4.10
    > Signed-off-by: Quinn Tran <qutran@marvell.com>
    > Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
    > ---
    >  drivers/scsi/qla2xxx/qla_attr.c | 3 ++-
    >  1 file changed, 2 insertions(+), 1 deletion(-)
    > 
    > diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
    > index 8d560c562e9c..0341f3340edb 100644
    > --- a/drivers/scsi/qla2xxx/qla_attr.c
    > +++ b/drivers/scsi/qla2xxx/qla_attr.c
    > @@ -275,7 +275,8 @@ qla2x00_sysfs_read_optrom(struct file *filp, struct kobject *kobj,
    >  
    >  	mutex_lock(&ha->optrom_mutex);
    >  
    > -	if (ha->optrom_state != QLA_SREADING)
    > +	if ((ha->optrom_state != QLA_SREADING) ||
    > +	    !buf || !ha->optrom_buffer)
    >  		goto out;
    >  
    
    In this case it appears as if no error is returned, since rval is always 0?
    Presumably this is the intentional behavior if it's not in the SREADING state?
    
    Why is the test for !buf necessary, since such a check is mostly not done here
    except for qlini_mode_store().  Is that the actual null pointer encountered?
    I don't see how ha->optrom_buffer would ever be NULL in the QLA_SREADING state.
    How does an optrom write failure result in this?  
    
    -Ewan

We found out that customers code base was missing patch by Milan which resolves
same stack trace and crash. 

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c7702b8c22712a06080e10f1d2dee1a133ec8809

We asked customer to pull that patch to resolve their crash.

I’ll drop this patch from the queue. 

Thanks,
Himanshu
    
    >  	rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
diff mbox series

Patch

diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index 8d560c562e9c..0341f3340edb 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -275,7 +275,8 @@  qla2x00_sysfs_read_optrom(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&ha->optrom_mutex);
 
-	if (ha->optrom_state != QLA_SREADING)
+	if ((ha->optrom_state != QLA_SREADING) ||
+	    !buf || !ha->optrom_buffer)
 		goto out;
 
 	rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,