linux-kernel - Issue with sg_copy_to_buffer() ? (was Re: [PATCH v3 2/4] crypto: talitos

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] [day] [month] [year] [list]
Message-ID: <0cbec7fa-445c-4644-5ea9-9f0f774b20fd@c-s.fr>
Date:   Sat, 15 Jun 2019 10:37:23 +0200
From:   Christophe Leroy <christophe.leroy@....fr>
To:     Horia Geanta <horia.geanta@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>
Cc:     "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>
Subject: Issue with sg_copy_to_buffer() ? (was Re: [PATCH v3 2/4] crypto:
 talitos - fix hash on SEC1.)



Le 15/06/2019 à 10:18, Christophe Leroy a écrit :
>>> @@ -2058,7 +2065,18 @@ static int ahash_process_req(struct 
>>> ahash_request *areq, unsigned int nbytes)
>>>           sg_copy_to_buffer(areq->src, nents,
>>>                     ctx_buf + req_ctx->nbuf, offset);
>>>           req_ctx->nbuf += offset;
>>> -        req_ctx->psrc = areq->src;
>>> +        for (sg = areq->src; sg && offset >= sg->length;
>>> +             offset -= sg->length, sg = sg_next(sg))
>>> +            ;
>>> +        if (offset) {
>>> +            sg_init_table(req_ctx->bufsl, 2);
>>> +            sg_set_buf(req_ctx->bufsl, sg_virt(sg) + offset,
>>> +                   sg->length - offset);
>>> +            sg_chain(req_ctx->bufsl, 2, sg_next(sg));
>>> +            req_ctx->psrc = req_ctx->bufsl;
>> Isn't this what scatterwalk_ffwd() does?
> 
> Thanks for pointing this, I wasn't aware of that function. Looking at it 
> it seems to do the same. Unfortunately, some tests fails with 'wrong 
> result' when using it instead.
> 
> Comparing the results of scatterwalk_ffwd() with what I get with my open 
> codying, I see the following difference:
> 
> scatterwalk_ffwd() uses sg_page(sg) + sg->offset + len
> 
> while my open codying results in virt_to_page(sg_virt(sg) + len)
> 
> When sg->offset + len is greater than PAGE_SIZE, the resulting SG entry 
> is different allthough valid in both cases. I think this difference 
> results in sg_copy_to_buffer() failing. I'm still investigating. Any idea ?
> 

After adding some dumps, I confirm the suspicion:

My board uses 16k pages.

SG list when not using scatterwalk_ffwd()
[   64.120540] sg c6386794 page c7fc1c60 offset 22 len 213
[   64.120579] sg c6386a48 page c7fc1b80 offset 4 len 2
[   64.120618] sg c6386a5c page c7fc1b00 offset 3 len 17
[   64.120658] sg c6386a70 page c7fc1b40 offset 2 len 10

SG list when using scatterwalk_ffwd()
[   64.120743] sg c6386794 page c7fc1c40 offset 16406 len 213
[   64.120782] sg c6386a48 page c7fc1b80 offset 4 len 2
[   64.120821] sg c6386a5c page c7fc1b00 offset 3 len 17
[   64.120861] sg c6386a70 page c7fc1b40 offset 2 len 10

Content of the SG list:
[   64.120975] 00000000: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 b8 10
[   64.121021] 00000010: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.121067] 00000020: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 b8 10
[   64.121112] 00000030: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.121157] 00000040: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 b8 10
[   64.121202] 00000050: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.121247] 00000060: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 a8 10
[   64.121292] 00000070: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.121337] 00000080: e8 40 98 f0 48 a0 f8 50 28 00 58 b0 08 60 b8 10
[   64.121382] 00000090: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.121427] 000000a0: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 b8 10
[   64.121472] 000000b0: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.121517] 000000c0: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 b8 10
[   64.121557] 000000d0: 68 c0 18 30 c8
[   64.121598] 00000000: 20 78
[   64.121646] 00000000: d0 28 80 f8 30 88 e0 38 90 e8 40 98 f0 48 a0 f8
[   64.121684] 00000010: 50
[   64.121729] 00000000: a8 00 58 b0 08 60 b8 10 68 c0

Content of the buffer after the copy from the list:
[   64.121790] 00000000: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 b8 10
[   64.121836] 00000010: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.121881] 00000020: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 b8 10
[   64.121927] 00000030: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.121972] 00000040: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 b8 10
[   64.122017] 00000050: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.122062] 00000060: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 a8 10
[   64.122107] 00000070: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.122152] 00000080: e8 40 98 f0 48 a0 f8 50 28 00 58 b0 08 60 b8 10
[   64.122197] 00000090: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.122243] 000000a0: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 b8 10
[   64.122288] 000000b0: 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90
[   64.122333] 000000c0: e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 60 b8 10
[   64.122378] 000000d0: 68 c0 18 30 c8 d8 b0 08 60 b8 10 68 c0 18 70 c8
[   64.122424] 000000e0: 20 78 d0 28 80 d8 30 88 e0 38 90 e8 40 98 f0 48
[   64.122462] 000000f0: a0 f8

As you can see, the data following the first block should be
20 78 d0 28 80 f8 30 88 e0 38 90 e8 40 98 f0 48 a0 f8 50 a8 00 58 b0 08 
60 b8 10 68 c0

Instead it is
d8 b0 08 60 b8 10 68 c0 18 70 c8 20 78 d0 28 80 d8 30 88 e0 38 90 e8 40 
98 f0 48 a0 f8

So I guess there is something wrong with sg_copy_to_buffer()

Christophe