linux-kernel - Re: usercopy whitelist woe in scsi_sense

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]

Date:   Tue, 17 Apr 2018 13:46:04 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Jens Axboe <axboe@...nel.dk>,
        Paolo Valente <paolo.valente@...aro.org>
Cc:     Oleksandr Natalenko <oleksandr@...alenko.name>,
        Bart Van Assche <bart.vanassche@....com>,
        David Windsor <dave@...lcore.net>,
        "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        linux-scsi@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        Christoph Hellwig <hch@....de>,
        Hannes Reinecke <hare@...e.com>,
        Johannes Thumshirn <jthumshirn@...e.de>,
        linux-block@...r.kernel.org
Subject: Re: usercopy whitelist woe in scsi_sense_cache

On Tue, Apr 17, 2018 at 1:28 PM, Jens Axboe <axboe@...nel.dk> wrote:
> It has to be the latter bfqq->dispatched increment, as those are
> transient (and bfqd is not).

Yeah, and I see a lot of comments around the lifetime of rq and bfqq,
so I assume something is not being locked correctly.

#define RQ_BFQQ(rq)             ((rq)->elv.priv[1])

static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
{
        struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
        struct request *rq = NULL;
        struct bfq_queue *bfqq = NULL;

        if (!list_empty(&bfqd->dispatch)) {
                rq = list_first_entry(&bfqd->dispatch, struct request,
                                      queuelist);
                list_del_init(&rq->queuelist);

                bfqq = RQ_BFQQ(rq);

                if (bfqq) {
                        /*
                         * Increment counters here, because this
                         * dispatch does not follow the standard
                         * dispatch flow (where counters are
                         * incremented)
                         */
                        bfqq->dispatched++;
...

I see elv.priv[1] assignments made in a few places -- is it possible
there is some kind of uninitialized-but-not-NULL state that can leak
in there?

bfq_prepare_request() assigns elv.priv[1], and bfq_insert_request()
only checks that it's non-NULL (if at all) in one case. Can
bfq_insert_request() get called without bfq_prepare_request() being
called first?

-Kees

-- 
Kees Cook
Pixel Security