linux-kernel - Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <4b411836-e76f-b67a-3d49-ad3d51b8f216@linux.vnet.ibm.com>
Date:   Fri, 28 Oct 2016 11:53:46 +0200
From:   Steffen Maier <maier@...ux.vnet.ibm.com>
To:     Johannes Thumshirn <jthumshirn@...e.de>
Cc:     "Martin K . Petersen" <martin.petersen@...cle.com>,
        Christoph Hellwig <hch@...radead.org>,
        Hannes Reinecke <hare@...e.de>,
        Linux Kernel Mailinglist <linux-kernel@...r.kernel.org>,
        Linux SCSI Mailinglist <linux-scsi@...r.kernel.org>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        Heiko Carstens <heiko.carstens@...ibm.com>,
        Anil Gurumurthy <anil.gurumurthy@...gic.com>,
        Sudarsana Kalluru <sudarsana.kalluru@...gic.com>,
        "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
        Tyrel Datwyler <tyreld@...ux.vnet.ibm.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Johannes Thumshirn <jth@...nel.org>,
        James Smart <james.smart@...gotech.com>,
        Dick Kennedy <dick.kennedy@...gotech.com>,
        "supporter:QLOGIC QLA2XXX FC-SCSI DRIVER" 
        <qla2xxx-upstream@...gic.com>,
        "open list:S390 ZFCP DRIVER" <linux-s390@...r.kernel.org>,
        "open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)" 
        <linuxppc-dev@...ts.ozlabs.org>,
        "open list:FCOE SUBSYSTEM (libfc, libfcoe, fcoe)" 
        <fcoe-devel@...n-fcoe.org>
Subject: Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and
 fc_bsg_job::reply directly



On 10/13/2016 06:24 PM, Johannes Thumshirn wrote:
> On Thu, Oct 13, 2016 at 05:15:25PM +0200, Steffen Maier wrote:
>> I'm puzzled.
>>
>> $ git bisect start fc_bsg master

>>> 3087864ce3d7282f59021245d8a5f83ef1caef18 is the first bad commit
>>> commit 3087864ce3d7282f59021245d8a5f83ef1caef18
>>> Author: Johannes Thumshirn <jthumshirn@...e.de>
>>> Date:   Wed Oct 12 15:06:28 2016 +0200
>>>
>>>     scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly
>>>
>>>     Don't use fc_bsg_job::request and fc_bsg_job::reply directly, but use
>>>     helper variables bsg_request and bsg_reply. This will be helpfull  when
>>>     transitioning to bsg-lib.
>>>
>>>     Signed-off-by: Johannes Thumshirn <jthumshirn@...e.de>
>>>
>>> :040000 040000 140c4b6829d5cfaec4079716e0795f63f8bc3bd2 0d9fe225615679550be91fbd9f84c09ab1e280fc M	drivers
>>
>> From there (on the reverse bisect path) I get the following Oops,
>> except for the full patch set having another stack trace as in my previous
>> mail (dying in zfcp code).
>>
>
> [...]
>
>>
>>> @@ -3937,6 +3944,7 @@ fc_bsg_request_handler(struct request_queue *q, struct Scsi_Host *shost,
>>>  	struct request *req;
>>>  	struct fc_bsg_job *job;
>>>  	enum fc_dispatch_result ret;
>>> +	struct fc_bsg_reply *bsg_reply;
>>>
>>>  	if (!get_device(dev))
>>>  		return;
>>> @@ -3973,8 +3981,9 @@ fc_bsg_request_handler(struct request_queue *q, struct Scsi_Host *shost,
>>>  		/* check if we have the msgcode value at least */
>>>  		if (job->request_len < sizeof(uint32_t)) {
>>>  			BUG_ON(job->reply_len < sizeof(uint32_t));
>>> -			job->reply->reply_payload_rcv_len = 0;
>>> -			job->reply->result = -ENOMSG;
>>> +			bsg_reply = job->reply;
>>> +			bsg_reply->reply_payload_rcv_len = 0;
>>> +			bsg_reply->result = -ENOMSG;

Compiler optimization re-ordered above two lines and the first pointer 
derefence is bsg_reply->result [field offset 0] where bsg_reply is NULL.
The assignment tries to write to memory at address NULL causing the 
kernel page fault.

Does your suggested change for [PATCH v3 02/16], shuffling the 
job->request_len checks, address above kernel page fault?

>>>  			job->reply_len = sizeof(uint32_t);
>>>  			fc_bsg_jobdone(job);
>>>  			spin_lock_irq(q->queue_lock);
>>>
>
> Ahm and what exactly can break here? It's just assigning variables. Now
> I'm puzzled too.


-- 
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294