| Message ID | 3af1a88c2ca7c30fc7bff30fba7eb3f3a57f67ed.1528314074.git.joseph.salisbury@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390/zcrypt: Fix CCA and EP11 CPRB processing failure memory leak. | expand |
On 13.06.2018 18:12, Joseph Salisbury wrote: > From: Harald Freudenberger <freude@de.ibm.com> > > BugLink: http://bugs.launchpad.net/bugs/1775390 > > Tests showed, that the zcrypt device driver produces memory > leaks when a valid CCA or EP11 CPRB can't get delivered or has > a failure during processing within the zcrypt device driver. > > This happens when a invalid domain or adapter number is used > or the lower level software or hardware layers produce any > kind of failure during processing of the request. > > Only CPRBs send to CCA or EP11 cards can produce this memory > leak. The accelerator and the CPRBs processed by this type > of crypto card is not affected. > > The two fields message and private within the ap_message struct > are allocated with pulling the function code for the CPRB but > only freed when processing of the CPRB succeeds. So for example > an invalid domain or adapter field causes the processing to > fail, leaving these two memory areas allocated forever. > > Signed-off-by: Harald Freudenberger <freude@de.ibm.com> > Reviewed-by: Ingo Franzki <ifranzki@de.ibm.com> > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> > (cherry picked from commit 89a0c0ec0d2e3ce0ee9caa00f60c0c26ccf11c21) > Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > drivers/s390/crypto/ap_bus.h | 17 ++++++++---- > drivers/s390/crypto/zcrypt_api.c | 15 ++++++++--- > drivers/s390/crypto/zcrypt_msgtype6.c | 51 +++++++++++++---------------------- > 3 files changed, 43 insertions(+), 40 deletions(-) > > diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h > index e0827ea..4c82946 100644 > --- a/drivers/s390/crypto/ap_bus.h > +++ b/drivers/s390/crypto/ap_bus.h > @@ -198,11 +198,18 @@ struct ap_message { > */ > static inline void ap_init_message(struct ap_message *ap_msg) > { > - ap_msg->psmid = 0; > - ap_msg->length = 0; > - ap_msg->rc = 0; > - ap_msg->special = 0; > - ap_msg->receive = NULL; > + memset(ap_msg, 0, sizeof(*ap_msg)); > +} > + > +/** > + * ap_release_message() - Release ap_message. > + * Releases all memory used internal within the ap_message struct > + * Currently this is the message and private field. > + */ > +static inline void ap_release_message(struct ap_message *ap_msg) > +{ > + kzfree(ap_msg->message); > + kzfree(ap_msg->private); > } > > #define for_each_ap_card(_ac) \ > diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c > index ce15f10..9a8250d 100644 > --- a/drivers/s390/crypto/zcrypt_api.c > +++ b/drivers/s390/crypto/zcrypt_api.c > @@ -373,6 +373,7 @@ long zcrypt_send_cprb(struct ica_xcRB *xcRB) > > trace_s390_zcrypt_req(xcRB, TB_ZSECSENDCPRB); > > + ap_init_message(&ap_msg); > rc = get_cprb_fc(xcRB, &ap_msg, &func_code, &domain); > if (rc) > goto out; > @@ -427,6 +428,7 @@ long zcrypt_send_cprb(struct ica_xcRB *xcRB) > spin_unlock(&zcrypt_list_lock); > > out: > + ap_release_message(&ap_msg); > trace_s390_zcrypt_rep(xcRB, func_code, rc, > AP_QID_CARD(qid), AP_QID_QUEUE(qid)); > return rc; > @@ -470,6 +472,8 @@ static long zcrypt_send_ep11_cprb(struct ep11_urb *xcrb) > > trace_s390_zcrypt_req(xcrb, TP_ZSENDEP11CPRB); > > + ap_init_message(&ap_msg); > + > target_num = (unsigned short) xcrb->targets_num; > > /* empty list indicates autoselect (all available targets) */ > @@ -487,7 +491,7 @@ static long zcrypt_send_ep11_cprb(struct ep11_urb *xcrb) > if (copy_from_user(targets, uptr, > target_num * sizeof(*targets))) { > rc = -EFAULT; > - goto out; > + goto out_free; > } > } > > @@ -544,6 +548,7 @@ static long zcrypt_send_ep11_cprb(struct ep11_urb *xcrb) > out_free: > kfree(targets); > out: > + ap_release_message(&ap_msg); > trace_s390_zcrypt_rep(xcrb, func_code, rc, > AP_QID_CARD(qid), AP_QID_QUEUE(qid)); > return rc; > @@ -561,6 +566,7 @@ static long zcrypt_rng(char *buffer) > > trace_s390_zcrypt_req(buffer, TP_HWRNGCPRB); > > + ap_init_message(&ap_msg); > rc = get_rng_fc(&ap_msg, &func_code, &domain); > if (rc) > goto out; > @@ -591,8 +597,10 @@ static long zcrypt_rng(char *buffer) > pref_zq = zcrypt_pick_queue(pref_zc, pref_zq, weight); > spin_unlock(&zcrypt_list_lock); > > - if (!pref_zq) > - return -ENODEV; > + if (!pref_zq) { > + rc = -ENODEV; > + goto out; > + } > > qid = pref_zq->queue->qid; > rc = pref_zq->ops->rng(pref_zq, buffer, &ap_msg); > @@ -602,6 +610,7 @@ static long zcrypt_rng(char *buffer) > spin_unlock(&zcrypt_list_lock); > > out: > + ap_release_message(&ap_msg); > trace_s390_zcrypt_rep(buffer, func_code, rc, > AP_QID_CARD(qid), AP_QID_QUEUE(qid)); > return rc; > diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c > index f54bef4..97d4bac 100644 > --- a/drivers/s390/crypto/zcrypt_msgtype6.c > +++ b/drivers/s390/crypto/zcrypt_msgtype6.c > @@ -1084,6 +1084,13 @@ static long zcrypt_msgtype6_modexpo_crt(struct zcrypt_queue *zq, > return rc; > } > > +/** > + * Fetch function code from cprb. > + * Extracting the fc requires to copy the cprb from userspace. > + * So this function allocates memory and needs an ap_msg prepared > + * by the caller with ap_init_message(). Also the caller has to > + * make sure ap_release_message() is always called even on failure. > + */ > unsigned int get_cprb_fc(struct ica_xcRB *xcRB, > struct ap_message *ap_msg, > unsigned int *func_code, unsigned short **dom) > @@ -1091,9 +1098,7 @@ unsigned int get_cprb_fc(struct ica_xcRB *xcRB, > struct response_type resp_type = { > .type = PCIXCC_RESPONSE_TYPE_XCRB, > }; > - int rc; > > - ap_init_message(ap_msg); > ap_msg->message = kmalloc(MSGTYPE06_MAX_MSG_SIZE, GFP_KERNEL); > if (!ap_msg->message) > return -ENOMEM; > @@ -1101,17 +1106,10 @@ unsigned int get_cprb_fc(struct ica_xcRB *xcRB, > ap_msg->psmid = (((unsigned long long) current->pid) << 32) + > atomic_inc_return(&zcrypt_step); > ap_msg->private = kmalloc(sizeof(resp_type), GFP_KERNEL); > - if (!ap_msg->private) { > - kzfree(ap_msg->message); > + if (!ap_msg->private) > return -ENOMEM; > - } > memcpy(ap_msg->private, &resp_type, sizeof(resp_type)); > - rc = XCRB_msg_to_type6CPRB_msgX(ap_msg, xcRB, func_code, dom); > - if (rc) { > - kzfree(ap_msg->message); > - kzfree(ap_msg->private); > - } > - return rc; > + return XCRB_msg_to_type6CPRB_msgX(ap_msg, xcRB, func_code, dom); > } > > /** > @@ -1139,11 +1137,16 @@ static long zcrypt_msgtype6_send_cprb(struct zcrypt_queue *zq, > /* Signal pending. */ > ap_cancel_message(zq->queue, ap_msg); > > - kzfree(ap_msg->message); > - kzfree(ap_msg->private); > return rc; > } > > +/** > + * Fetch function code from ep11 cprb. > + * Extracting the fc requires to copy the ep11 cprb from userspace. > + * So this function allocates memory and needs an ap_msg prepared > + * by the caller with ap_init_message(). Also the caller has to > + * make sure ap_release_message() is always called even on failure. > + */ > unsigned int get_ep11cprb_fc(struct ep11_urb *xcrb, > struct ap_message *ap_msg, > unsigned int *func_code) > @@ -1151,9 +1154,7 @@ unsigned int get_ep11cprb_fc(struct ep11_urb *xcrb, > struct response_type resp_type = { > .type = PCIXCC_RESPONSE_TYPE_EP11, > }; > - int rc; > > - ap_init_message(ap_msg); > ap_msg->message = kmalloc(MSGTYPE06_MAX_MSG_SIZE, GFP_KERNEL); > if (!ap_msg->message) > return -ENOMEM; > @@ -1161,17 +1162,10 @@ unsigned int get_ep11cprb_fc(struct ep11_urb *xcrb, > ap_msg->psmid = (((unsigned long long) current->pid) << 32) + > atomic_inc_return(&zcrypt_step); > ap_msg->private = kmalloc(sizeof(resp_type), GFP_KERNEL); > - if (!ap_msg->private) { > - kzfree(ap_msg->message); > + if (!ap_msg->private) > return -ENOMEM; > - } > memcpy(ap_msg->private, &resp_type, sizeof(resp_type)); > - rc = xcrb_msg_to_type6_ep11cprb_msgx(ap_msg, xcrb, func_code); > - if (rc) { > - kzfree(ap_msg->message); > - kzfree(ap_msg->private); > - } > - return rc; > + return xcrb_msg_to_type6_ep11cprb_msgx(ap_msg, xcrb, func_code); > } > > /** > @@ -1246,8 +1240,6 @@ static long zcrypt_msgtype6_send_ep11_cprb(struct zcrypt_queue *zq, > /* Signal pending. */ > ap_cancel_message(zq->queue, ap_msg); > > - kzfree(ap_msg->message); > - kzfree(ap_msg->private); > return rc; > } > > @@ -1258,7 +1250,6 @@ unsigned int get_rng_fc(struct ap_message *ap_msg, int *func_code, > .type = PCIXCC_RESPONSE_TYPE_XCRB, > }; > > - ap_init_message(ap_msg); > ap_msg->message = kmalloc(MSGTYPE06_MAX_MSG_SIZE, GFP_KERNEL); > if (!ap_msg->message) > return -ENOMEM; > @@ -1266,10 +1257,8 @@ unsigned int get_rng_fc(struct ap_message *ap_msg, int *func_code, > ap_msg->psmid = (((unsigned long long) current->pid) << 32) + > atomic_inc_return(&zcrypt_step); > ap_msg->private = kmalloc(sizeof(resp_type), GFP_KERNEL); > - if (!ap_msg->private) { > - kzfree(ap_msg->message); > + if (!ap_msg->private) > return -ENOMEM; > - } > memcpy(ap_msg->private, &resp_type, sizeof(resp_type)); > > rng_type6CPRB_msgX(ap_msg, ZCRYPT_RNG_BUFFER_SIZE, domain); > @@ -1313,8 +1302,6 @@ static long zcrypt_msgtype6_rng(struct zcrypt_queue *zq, > /* Signal pending. */ > ap_cancel_message(zq->queue, ap_msg); > > - kzfree(ap_msg->message); > - kzfree(ap_msg->private); > return rc; > } > >
Applied to unstable master branch. Thanks. Cascardo. Applied-to: unstable/master
diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h index e0827ea..4c82946 100644 --- a/drivers/s390/crypto/ap_bus.h +++ b/drivers/s390/crypto/ap_bus.h @@ -198,11 +198,18 @@ struct ap_message { */ static inline void ap_init_message(struct ap_message *ap_msg) { - ap_msg->psmid = 0; - ap_msg->length = 0; - ap_msg->rc = 0; - ap_msg->special = 0; - ap_msg->receive = NULL; + memset(ap_msg, 0, sizeof(*ap_msg)); +} + +/** + * ap_release_message() - Release ap_message. + * Releases all memory used internal within the ap_message struct + * Currently this is the message and private field. + */ +static inline void ap_release_message(struct ap_message *ap_msg) +{ + kzfree(ap_msg->message); + kzfree(ap_msg->private); } #define for_each_ap_card(_ac) \ diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c index ce15f10..9a8250d 100644 --- a/drivers/s390/crypto/zcrypt_api.c +++ b/drivers/s390/crypto/zcrypt_api.c @@ -373,6 +373,7 @@ long zcrypt_send_cprb(struct ica_xcRB *xcRB) trace_s390_zcrypt_req(xcRB, TB_ZSECSENDCPRB); + ap_init_message(&ap_msg); rc = get_cprb_fc(xcRB, &ap_msg, &func_code, &domain); if (rc) goto out; @@ -427,6 +428,7 @@ long zcrypt_send_cprb(struct ica_xcRB *xcRB) spin_unlock(&zcrypt_list_lock); out: + ap_release_message(&ap_msg); trace_s390_zcrypt_rep(xcRB, func_code, rc, AP_QID_CARD(qid), AP_QID_QUEUE(qid)); return rc; @@ -470,6 +472,8 @@ static long zcrypt_send_ep11_cprb(struct ep11_urb *xcrb) trace_s390_zcrypt_req(xcrb, TP_ZSENDEP11CPRB); + ap_init_message(&ap_msg); + target_num = (unsigned short) xcrb->targets_num; /* empty list indicates autoselect (all available targets) */ @@ -487,7 +491,7 @@ static long zcrypt_send_ep11_cprb(struct ep11_urb *xcrb) if (copy_from_user(targets, uptr, target_num * sizeof(*targets))) { rc = -EFAULT; - goto out; + goto out_free; } } @@ -544,6 +548,7 @@ static long zcrypt_send_ep11_cprb(struct ep11_urb *xcrb) out_free: kfree(targets); out: + ap_release_message(&ap_msg); trace_s390_zcrypt_rep(xcrb, func_code, rc, AP_QID_CARD(qid), AP_QID_QUEUE(qid)); return rc; @@ -561,6 +566,7 @@ static long zcrypt_rng(char *buffer) trace_s390_zcrypt_req(buffer, TP_HWRNGCPRB); + ap_init_message(&ap_msg); rc = get_rng_fc(&ap_msg, &func_code, &domain); if (rc) goto out; @@ -591,8 +597,10 @@ static long zcrypt_rng(char *buffer) pref_zq = zcrypt_pick_queue(pref_zc, pref_zq, weight); spin_unlock(&zcrypt_list_lock); - if (!pref_zq) - return -ENODEV; + if (!pref_zq) { + rc = -ENODEV; + goto out; + } qid = pref_zq->queue->qid; rc = pref_zq->ops->rng(pref_zq, buffer, &ap_msg); @@ -602,6 +610,7 @@ static long zcrypt_rng(char *buffer) spin_unlock(&zcrypt_list_lock); out: + ap_release_message(&ap_msg); trace_s390_zcrypt_rep(buffer, func_code, rc, AP_QID_CARD(qid), AP_QID_QUEUE(qid)); return rc; diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c index f54bef4..97d4bac 100644 --- a/drivers/s390/crypto/zcrypt_msgtype6.c +++ b/drivers/s390/crypto/zcrypt_msgtype6.c @@ -1084,6 +1084,13 @@ static long zcrypt_msgtype6_modexpo_crt(struct zcrypt_queue *zq, return rc; } +/** + * Fetch function code from cprb. + * Extracting the fc requires to copy the cprb from userspace. + * So this function allocates memory and needs an ap_msg prepared + * by the caller with ap_init_message(). Also the caller has to + * make sure ap_release_message() is always called even on failure. + */ unsigned int get_cprb_fc(struct ica_xcRB *xcRB, struct ap_message *ap_msg, unsigned int *func_code, unsigned short **dom) @@ -1091,9 +1098,7 @@ unsigned int get_cprb_fc(struct ica_xcRB *xcRB, struct response_type resp_type = { .type = PCIXCC_RESPONSE_TYPE_XCRB, }; - int rc; - ap_init_message(ap_msg); ap_msg->message = kmalloc(MSGTYPE06_MAX_MSG_SIZE, GFP_KERNEL); if (!ap_msg->message) return -ENOMEM; @@ -1101,17 +1106,10 @@ unsigned int get_cprb_fc(struct ica_xcRB *xcRB, ap_msg->psmid = (((unsigned long long) current->pid) << 32) + atomic_inc_return(&zcrypt_step); ap_msg->private = kmalloc(sizeof(resp_type), GFP_KERNEL); - if (!ap_msg->private) { - kzfree(ap_msg->message); + if (!ap_msg->private) return -ENOMEM; - } memcpy(ap_msg->private, &resp_type, sizeof(resp_type)); - rc = XCRB_msg_to_type6CPRB_msgX(ap_msg, xcRB, func_code, dom); - if (rc) { - kzfree(ap_msg->message); - kzfree(ap_msg->private); - } - return rc; + return XCRB_msg_to_type6CPRB_msgX(ap_msg, xcRB, func_code, dom); } /** @@ -1139,11 +1137,16 @@ static long zcrypt_msgtype6_send_cprb(struct zcrypt_queue *zq, /* Signal pending. */ ap_cancel_message(zq->queue, ap_msg); - kzfree(ap_msg->message); - kzfree(ap_msg->private); return rc; } +/** + * Fetch function code from ep11 cprb. + * Extracting the fc requires to copy the ep11 cprb from userspace. + * So this function allocates memory and needs an ap_msg prepared + * by the caller with ap_init_message(). Also the caller has to + * make sure ap_release_message() is always called even on failure. + */ unsigned int get_ep11cprb_fc(struct ep11_urb *xcrb, struct ap_message *ap_msg, unsigned int *func_code) @@ -1151,9 +1154,7 @@ unsigned int get_ep11cprb_fc(struct ep11_urb *xcrb, struct response_type resp_type = { .type = PCIXCC_RESPONSE_TYPE_EP11, }; - int rc; - ap_init_message(ap_msg); ap_msg->message = kmalloc(MSGTYPE06_MAX_MSG_SIZE, GFP_KERNEL); if (!ap_msg->message) return -ENOMEM; @@ -1161,17 +1162,10 @@ unsigned int get_ep11cprb_fc(struct ep11_urb *xcrb, ap_msg->psmid = (((unsigned long long) current->pid) << 32) + atomic_inc_return(&zcrypt_step); ap_msg->private = kmalloc(sizeof(resp_type), GFP_KERNEL); - if (!ap_msg->private) { - kzfree(ap_msg->message); + if (!ap_msg->private) return -ENOMEM; - } memcpy(ap_msg->private, &resp_type, sizeof(resp_type)); - rc = xcrb_msg_to_type6_ep11cprb_msgx(ap_msg, xcrb, func_code); - if (rc) { - kzfree(ap_msg->message); - kzfree(ap_msg->private); - } - return rc; + return xcrb_msg_to_type6_ep11cprb_msgx(ap_msg, xcrb, func_code); } /** @@ -1246,8 +1240,6 @@ static long zcrypt_msgtype6_send_ep11_cprb(struct zcrypt_queue *zq, /* Signal pending. */ ap_cancel_message(zq->queue, ap_msg); - kzfree(ap_msg->message); - kzfree(ap_msg->private); return rc; } @@ -1258,7 +1250,6 @@ unsigned int get_rng_fc(struct ap_message *ap_msg, int *func_code, .type = PCIXCC_RESPONSE_TYPE_XCRB, }; - ap_init_message(ap_msg); ap_msg->message = kmalloc(MSGTYPE06_MAX_MSG_SIZE, GFP_KERNEL); if (!ap_msg->message) return -ENOMEM; @@ -1266,10 +1257,8 @@ unsigned int get_rng_fc(struct ap_message *ap_msg, int *func_code, ap_msg->psmid = (((unsigned long long) current->pid) << 32) + atomic_inc_return(&zcrypt_step); ap_msg->private = kmalloc(sizeof(resp_type), GFP_KERNEL); - if (!ap_msg->private) { - kzfree(ap_msg->message); + if (!ap_msg->private) return -ENOMEM; - } memcpy(ap_msg->private, &resp_type, sizeof(resp_type)); rng_type6CPRB_msgX(ap_msg, ZCRYPT_RNG_BUFFER_SIZE, domain); @@ -1313,8 +1302,6 @@ static long zcrypt_msgtype6_rng(struct zcrypt_queue *zq, /* Signal pending. */ ap_cancel_message(zq->queue, ap_msg); - kzfree(ap_msg->message); - kzfree(ap_msg->private); return rc; }