[PATCH 1/1] crypto: Fix possible buffer overflows in pkey_protkey_aes_attr_read
From: Zhang Xiaohui pkey_protkey_aes_attr_read() calls memcpy() without checking the destination size may trigger a buffer overflower. Signed-off-by: Zhang Xiaohui --- drivers/s390/crypto/pkey_api.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c index 99cb60ea6..abc237130 100644 --- a/drivers/s390/crypto/pkey_api.c +++ b/drivers/s390/crypto/pkey_api.c @@ -1589,6 +1589,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, bool is_xts, char *buf, if (rc) return rc; + if (protkey.len > MAXPROTKEYSIZE) + protkey.len = MAXPROTKEYSIZE; protkeytoken.len = protkey.len; memcpy(, , protkey.len); @@ -1599,6 +1601,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, bool is_xts, char *buf, if (rc) return rc; + if (protkey.len > MAXPROTKEYSIZE) + protkey.len = MAXPROTKEYSIZE; protkeytoken.len = protkey.len; memcpy(, , protkey.len); -- 2.17.1
Re: [PATCH 1/1] crypto: Fix possible buffer overflows in pkey_protkey_aes_attr_read
On 09.12.20 07:47, Xiaohui Zhang wrote: > From: Zhang Xiaohui > > pkey_protkey_aes_attr_read() calls memcpy() without checking the > destination size may trigger a buffer overflower. > > Signed-off-by: Zhang Xiaohui > --- > drivers/s390/crypto/pkey_api.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c > index 99cb60ea6..abc237130 100644 > --- a/drivers/s390/crypto/pkey_api.c > +++ b/drivers/s390/crypto/pkey_api.c > @@ -1589,6 +1589,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, > bool is_xts, char *buf, > if (rc) > return rc; > > + if (protkey.len > MAXPROTKEYSIZE) > + protkey.len = MAXPROTKEYSIZE; > protkeytoken.len = protkey.len; > memcpy(, , protkey.len); > > @@ -1599,6 +1601,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, > bool is_xts, char *buf, > if (rc) > return rc; > > + if (protkey.len > MAXPROTKEYSIZE) > + protkey.len = MAXPROTKEYSIZE; > protkeytoken.len = protkey.len; > memcpy(, , protkey.len); > Thanks Xiaohui but one rule within the kernel is to trust the other internal functions to do the right thing. So usually only on entrance into the kernel the api parameters are checked but within the kernel each function trusts the other and no further parameter check is done. Otherwise endless checks of input parameters would take place which is killing the performance. As you can see the protkey object is stored by the function pkey_genprotkey() which is called just 2 lines above. An internal function the module should trust here. I don't think there is an additional length check needed here. However, Thanks for your contribution. Harald Freudenberger see this function calls another function in the very same file and
Re: [PATCH 1/1] crypto: Fix possible buffer overflows in pkey_protkey_aes_attr_read
On 09.12.20 07:47, Xiaohui Zhang wrote: > From: Zhang Xiaohui > > pkey_protkey_aes_attr_read() calls memcpy() without checking the > destination size may trigger a buffer overflower. To me it looks like protkey.len is generated programmatically in pkey_genprotkey/pkey_clr2protkey and this purely depends on the keytype and we do check for known ones. Not sure how this can happen. > > Signed-off-by: Zhang Xiaohui > --- > drivers/s390/crypto/pkey_api.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c > index 99cb60ea6..abc237130 100644 > --- a/drivers/s390/crypto/pkey_api.c > +++ b/drivers/s390/crypto/pkey_api.c > @@ -1589,6 +1589,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, > bool is_xts, char *buf, > if (rc) > return rc; > > + if (protkey.len > MAXPROTKEYSIZE) > + protkey.len = MAXPROTKEYSIZE; > protkeytoken.len = protkey.len; > memcpy(, , protkey.len); > > @@ -1599,6 +1601,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, > bool is_xts, char *buf, > if (rc) > return rc; > > + if (protkey.len > MAXPROTKEYSIZE) > + protkey.len = MAXPROTKEYSIZE; > protkeytoken.len = protkey.len; > memcpy(, , protkey.len); > >
