From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42653) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgHt8-00052r-HR for qemu-devel@nongnu.org; Mon, 20 Oct 2014 14:41:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgHt4-0000T8-0N for qemu-devel@nongnu.org; Mon, 20 Oct 2014 14:41:02 -0400 Sender: Paolo Bonzini Message-ID: <5445572B.9060800@redhat.com> Date: Mon, 20 Oct 2014 20:40:43 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <544358DA.6040000@pobox.com> In-Reply-To: <544358DA.6040000@pobox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] libcacard: don't free sign buffer while sign op is pending List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alon Levy , Ray Strode , qemu-devel@nongnu.org Cc: Ray Strode , Michael Tokarev , qemu-stable@nongnu.org, Nalin Dahyabhai , =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= , Robert Relyea On 10/19/2014 08:23 AM, Alon Levy wrote: > On 10/19/2014 05:12 AM, Ray Strode wrote: >> From: Ray Strode >> >> commit 57f97834efe0c208ffadc9d2959f3d3d55580e52 cleaned up >> the cac_applet_pki_process_apdu function to have a single >> exit point. Unfortunately, that commit introduced a bug >> where the sign buffer can get free'd and nullified while >> it's still being used. >> >> This commit corrects the bug by introducing a boolean to >> track whether or not the sign buffer should be freed in >> the function exit path. > > My bad, thanks for catching this. > > Reviewed-by: Alon Levy Please Cc qemu-stable@nongnu.org if you send a pull request. Paolo > >> >> Signed-off-by: Ray Strode >> --- >> libcacard/cac.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/libcacard/cac.c b/libcacard/cac.c >> index ae8c378..f38fdce 100644 >> --- a/libcacard/cac.c >> +++ b/libcacard/cac.c >> @@ -88,60 +88,61 @@ cac_common_process_apdu(VCard *card, VCardAPDU *apdu, VCardResponse **response) >> } >> >> /* >> * reset the inter call state between applet selects >> */ >> static VCardStatus >> cac_applet_pki_reset(VCard *card, int channel) >> { >> VCardAppletPrivate *applet_private; >> CACPKIAppletData *pki_applet; >> applet_private = vcard_get_current_applet_private(card, channel); >> assert(applet_private); >> pki_applet = &(applet_private->u.pki_data); >> >> pki_applet->cert_buffer = NULL; >> g_free(pki_applet->sign_buffer); >> pki_applet->sign_buffer = NULL; >> pki_applet->cert_buffer_len = 0; >> pki_applet->sign_buffer_len = 0; >> return VCARD_DONE; >> } >> >> static VCardStatus >> cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu, >> VCardResponse **response) >> { >> CACPKIAppletData *pki_applet; >> VCardAppletPrivate *applet_private; >> int size, next; >> unsigned char *sign_buffer; >> + bool retain_sign_buffer = FALSE; >> vcard_7816_status_t status; >> VCardStatus ret = VCARD_FAIL; >> >> applet_private = vcard_get_current_applet_private(card, apdu->a_channel); >> assert(applet_private); >> pki_applet = &(applet_private->u.pki_data); >> >> switch (apdu->a_ins) { >> case CAC_UPDATE_BUFFER: >> *response = vcard_make_response( >> VCARD7816_STATUS_ERROR_CONDITION_NOT_SATISFIED); >> ret = VCARD_DONE; >> break; >> case CAC_GET_CERTIFICATE: >> if ((apdu->a_p2 != 0) || (apdu->a_p1 != 0)) { >> *response = vcard_make_response( >> VCARD7816_STATUS_ERROR_P1_P2_INCORRECT); >> break; >> } >> assert(pki_applet->cert != NULL); >> size = apdu->a_Le; >> if (pki_applet->cert_buffer == NULL) { >> pki_applet->cert_buffer = pki_applet->cert; >> pki_applet->cert_buffer_len = pki_applet->cert_len; >> } >> size = MIN(size, pki_applet->cert_buffer_len); >> next = MIN(255, pki_applet->cert_buffer_len - size); >> *response = vcard_response_new_bytes( >> card, pki_applet->cert_buffer, size, >> apdu->a_Le, next ? >> @@ -151,85 +152,88 @@ cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu, >> pki_applet->cert_buffer += size; >> pki_applet->cert_buffer_len -= size; >> if ((*response == NULL) || (next == 0)) { >> pki_applet->cert_buffer = NULL; >> } >> if (*response == NULL) { >> *response = vcard_make_response( >> VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE); >> } >> ret = VCARD_DONE; >> break; >> case CAC_SIGN_DECRYPT: >> if (apdu->a_p2 != 0) { >> *response = vcard_make_response( >> VCARD7816_STATUS_ERROR_P1_P2_INCORRECT); >> break; >> } >> size = apdu->a_Lc; >> >> sign_buffer = g_realloc(pki_applet->sign_buffer, >> pki_applet->sign_buffer_len + size); >> memcpy(sign_buffer+pki_applet->sign_buffer_len, apdu->a_body, size); >> size += pki_applet->sign_buffer_len; >> switch (apdu->a_p1) { >> case 0x80: >> /* p1 == 0x80 means we haven't yet sent the whole buffer, wait for >> * the rest */ >> pki_applet->sign_buffer = sign_buffer; >> pki_applet->sign_buffer_len = size; >> *response = vcard_make_response(VCARD7816_STATUS_SUCCESS); >> + retain_sign_buffer = TRUE; >> break; >> case 0x00: >> /* we now have the whole buffer, do the operation, result will be >> * in the sign_buffer */ >> status = vcard_emul_rsa_op(card, pki_applet->key, >> sign_buffer, size); >> if (status != VCARD7816_STATUS_SUCCESS) { >> *response = vcard_make_response(status); >> break; >> } >> *response = vcard_response_new(card, sign_buffer, size, apdu->a_Le, >> VCARD7816_STATUS_SUCCESS); >> if (*response == NULL) { >> *response = vcard_make_response( >> VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE); >> } >> break; >> default: >> *response = vcard_make_response( >> VCARD7816_STATUS_ERROR_P1_P2_INCORRECT); >> break; >> } >> - g_free(sign_buffer); >> - pki_applet->sign_buffer = NULL; >> - pki_applet->sign_buffer_len = 0; >> + if (!retain_sign_buffer) { >> + g_free(sign_buffer); >> + pki_applet->sign_buffer = NULL; >> + pki_applet->sign_buffer_len = 0; >> + } >> ret = VCARD_DONE; >> break; >> case CAC_READ_BUFFER: >> /* new CAC call, go ahead and use the old version for now */ >> /* TODO: implement */ >> *response = vcard_make_response( >> VCARD7816_STATUS_ERROR_COMMAND_NOT_SUPPORTED); >> ret = VCARD_DONE; >> break; >> default: >> ret = cac_common_process_apdu(card, apdu, response); >> break; >> } >> return ret; >> } >> >> >> static VCardStatus >> cac_applet_id_process_apdu(VCard *card, VCardAPDU *apdu, >> VCardResponse **response) >> { >> VCardStatus ret = VCARD_FAIL; >> >> switch (apdu->a_ins) { >> case CAC_UPDATE_BUFFER: >> *response = vcard_make_response( >> VCARD7816_STATUS_ERROR_CONDITION_NOT_SATISFIED); >> ret = VCARD_DONE; >> break; >> case CAC_READ_BUFFER: >> > > >