[PATCH] crypto: ccp - zero the cmd data after use it

[PATCH] crypto: ccp - zero the cmd data after use it

[Liwei Song]

exist the following assignment in ccp(ignore the force
convert of the struct) by list_del in ccp_dequeue_cmd():
req->__ctx->cmd->entry->next = LIST_POISON1;

after use the req, kzfree(req) can not zero the entry
entry->next = LIST_POISON1 of the ccp_cmd(cmd) struct
when this address available as slub freelist pointer, this will cause
the following "general protection fault" error if some process meet
this LIST_POISON1 value address when request memory:

general protection fault: 0000 1 PREEMPT SMP NOPTI
CPU: 13 PID: 111282 Comm: msgstress03 Not tainted 5.2.45-yocto-standard #1
Hardware name: AMD Corporation Wallaby/Wallaby, BIOS WWB7713N 07/11/2017
RIP: 0010:__kmalloc_node+0x106/0x2f0
RSP: 0018:ffffaa6dd83ffdc8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 000000000033e0cd
RDX: 000000000033e08d RSI: 000000000033e08d RDI: 000000000002c180
RBP: ffffaa6dd83ffe00 R08: 00000000000000d4 R09: ffff966c9dc07180
R10: dead000000000100 R11: 0000000000000000 R12: 0000000000000cc0
R13: 0000000000000100 R14: 00000000ffffffff R15: ffff966c9dc07180
FS: 00007f83bb756600(0000) GS:ffff966c9e340000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f83bb6917e0 CR3: 000000080b794000 CR4: 00000000003406e0
Call Trace:
? kvmalloc_node+0x7b/0x90
kvmalloc_node+0x7b/0x90
newque+0x32/0x1a0
ipcget+0x27a/0x2c0
ksys_msgget+0x51/0x70
__x64_sys_msgget+0x16/0x20
do_syscall_64+0x4d/0x130
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f83bb6917e7

Fix it by zero cmd struct after finished use it.

Signed-off-by: Liwei Song <liwei.song@windriver.com>
---
 drivers/crypto/ccp/ccp-dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/ccp/ccp-dev.c b/drivers/crypto/ccp/ccp-dev.c
index edefa669153f..75a6418d541d 100644
--- a/drivers/crypto/ccp/ccp-dev.c
+++ b/drivers/crypto/ccp/ccp-dev.c
@@ -409,6 +409,7 @@ static void ccp_do_cmd_complete(unsigned long data)
 	cmd->callback(cmd->data, cmd->ret);
 
 	complete(&tdata->completion);
+	memset(cmd, 0, sizeof(*cmd));
 }
 
 /**
-- 
2.17.1

Re: [PATCH] crypto: ccp - zero the cmd data after use it

[Herbert Xu]

On Mon, Aug 03, 2020 at 03:58:58PM +0800, Liwei Song wrote:
> exist the following assignment in ccp(ignore the force
> convert of the struct) by list_del in ccp_dequeue_cmd():
> req->__ctx->cmd->entry->next = LIST_POISON1;
> 
> after use the req, kzfree(req) can not zero the entry
> entry->next = LIST_POISON1 of the ccp_cmd(cmd) struct
> when this address available as slub freelist pointer, this will cause
> the following "general protection fault" error if some process meet
> this LIST_POISON1 value address when request memory:

Your description makes no sense.  Please rewrite it and explain
the problem properly.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: ccp - zero the cmd data after use it

[Liwei Song]

On 8/3/20 20:52, Herbert Xu wrote:
> On Mon, Aug 03, 2020 at 03:58:58PM +0800, Liwei Song wrote:
>> exist the following assignment in ccp(ignore the force
>> convert of the struct) by list_del in ccp_dequeue_cmd():
>> req->__ctx->cmd->entry->next = LIST_POISON1;
>>
>> after use the req, kzfree(req) can not zero the entry
>> entry->next = LIST_POISON1 of the ccp_cmd(cmd) struct
>> when this address available as slub freelist pointer, this will cause
>> the following "general protection fault" error if some process meet
>> this LIST_POISON1 value address when request memory:
> 
> Your description makes no sense.  Please rewrite it and explain
> the problem properly.

The problem here is that the entry of struct ccp_cmd is not zeroed after we use it,
If the other process got this address by kmalloc(), this illegal value "LIST_POISON1"
will cause "general protection fault" error.

Thanks,
Liwei.


> 
> Thanks,
> 

Re: [PATCH] crypto: ccp - zero the cmd data after use it

[Herbert Xu]

On Tue, Aug 04, 2020 at 11:51:47AM +0800, Liwei Song wrote:
> 
> On 8/3/20 20:52, Herbert Xu wrote:
> > On Mon, Aug 03, 2020 at 03:58:58PM +0800, Liwei Song wrote:
> >> exist the following assignment in ccp(ignore the force
> >> convert of the struct) by list_del in ccp_dequeue_cmd():
> >> req->__ctx->cmd->entry->next = LIST_POISON1;
> >>
> >> after use the req, kzfree(req) can not zero the entry
> >> entry->next = LIST_POISON1 of the ccp_cmd(cmd) struct
> >> when this address available as slub freelist pointer, this will cause
> >> the following "general protection fault" error if some process meet
> >> this LIST_POISON1 value address when request memory:
> > 
> > Your description makes no sense.  Please rewrite it and explain
> > the problem properly.
> 
> The problem here is that the entry of struct ccp_cmd is not zeroed after we use it,
> If the other process got this address by kmalloc(), this illegal value "LIST_POISON1"
> will cause "general protection fault" error.

If that's the case surely the other process should be zeroing
the memory? Your explanation still makes no sense.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: ccp - zero the cmd data after use it

[Liwei Song]

On 8/4/20 12:04, Herbert Xu wrote:
> On Tue, Aug 04, 2020 at 11:51:47AM +0800, Liwei Song wrote:
>>
>> On 8/3/20 20:52, Herbert Xu wrote:
>>> On Mon, Aug 03, 2020 at 03:58:58PM +0800, Liwei Song wrote:
>>>> exist the following assignment in ccp(ignore the force
>>>> convert of the struct) by list_del in ccp_dequeue_cmd():
>>>> req->__ctx->cmd->entry->next = LIST_POISON1;
>>>>
>>>> after use the req, kzfree(req) can not zero the entry
>>>> entry->next = LIST_POISON1 of the ccp_cmd(cmd) struct
>>>> when this address available as slub freelist pointer, this will cause
>>>> the following "general protection fault" error if some process meet
>>>> this LIST_POISON1 value address when request memory:
>>>
>>> Your description makes no sense.  Please rewrite it and explain
>>> the problem properly.
>>
>> The problem here is that the entry of struct ccp_cmd is not zeroed after we use it,
>> If the other process got this address by kmalloc(), this illegal value "LIST_POISON1"
>> will cause "general protection fault" error.
> 
> If that's the case surely the other process should be zeroing
> the memory? Your explanation still makes no sense.

Yes, the other process should do this zero work, but the case I met is
this address will appear in the slab_alloc_node() as freelist pointer of slub,
and before slub do zero wrok, even kzalloc() doesn't work with this address.

Thanks,
Liwei.


> 
> Thanks,
> 

Re: [PATCH] crypto: ccp - zero the cmd data after use it

[Herbert Xu]

On Tue, Aug 04, 2020 at 12:20:21PM +0800, Liwei Song wrote:
>
> Yes, the other process should do this zero work, but the case I met is
> this address will appear in the slab_alloc_node() as freelist pointer of slub,
> and before slub do zero wrok, even kzalloc() doesn't work with this address.

That would be memory corruption which has nothing to do with your
patch.  If it is occurring then you should fix the place that is
corrupting the memory and not work around it like this.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: ccp - zero the cmd data after use it

[Liwei Song]

On 8/4/20 12:22, Herbert Xu wrote:
> On Tue, Aug 04, 2020 at 12:20:21PM +0800, Liwei Song wrote:
>>
>> Yes, the other process should do this zero work, but the case I met is
>> this address will appear in the slab_alloc_node() as freelist pointer of slub,
>> and before slub do zero wrok, even kzalloc() doesn't work with this address.
> 
> That would be memory corruption which has nothing to do with your
> patch.  If it is occurring then you should fix the place that is
> corrupting the memory and not work around it like this.

OK, understand, thanks for your suggestion.

Liwei.


> 
> Cheers,
>