From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 3/4] AMD/microcode: Avoid use-after-free for the microcode buffer Date: Fri, 27 Dec 2013 10:50:07 -0500 Message-ID: <52BDA1AF.6080202@oracle.com> References: <1388156253-14509-1-git-send-email-andrew.cooper3@citrix.com> <1388156253-14509-4-git-send-email-andrew.cooper3@citrix.com> <52BD9D24.30008@oracle.com> <52BD9E7B.5000708@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52BD9E7B.5000708@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Keir Fraser , Jan Beulich , Suravee Suthikulpanit , Xen-devel List-Id: xen-devel@lists.xenproject.org On 12/27/2013 10:36 AM, Andrew Cooper wrote: > On 27/12/2013 15:30, Boris Ostrovsky wrote: >> On 12/27/2013 09:57 AM, Andrew Cooper wrote: >>> It is possible to free the mc_old buffer and the store it for use on >>> in the >> I think you don't need *the* store here. And no *on*. > Oops - I refactored my sentence half way through, and missed that on a > reread. > >>> case of resume. >>> >>> This keeps the old semantics of being able to return an error even >>> after a >>> successful microcode application. >>> >>> Coverity-ID 1146953 >>> Signed-off-by: Andrew Cooper >>> CC: Keir Fraser >>> CC: Jan Beulich >>> CC: Boris Ostrovsky >>> CC: Suravee Suthikulpanit >>> --- >>> xen/arch/x86/microcode_amd.c | 17 +++++++++-------- >>> 1 file changed, 9 insertions(+), 8 deletions(-) >>> >>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c >>> index a3ceef8..8ea4e63 100644 >>> --- a/xen/arch/x86/microcode_amd.c >>> +++ b/xen/arch/x86/microcode_amd.c >>> @@ -275,7 +275,7 @@ static int cpu_request_microcode(int cpu, const >>> void *buf, size_t bufsize) >>> struct microcode_amd *mc_amd, *mc_old; >>> size_t offset = bufsize; >>> size_t last_offset, applied_offset = 0; >>> - int error = 0; >>> + int error = 0, save_error = 1; >>> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); >>> /* We should bind the task to the CPU */ >>> @@ -338,19 +338,20 @@ static int cpu_request_microcode(int cpu, const >>> void *buf, size_t bufsize) >>> */ >>> if ( applied_offset ) >>> { >>> - int ret = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, >>> - &applied_offset); >>> - if ( ret == 0 ) >>> - xfree(mc_old); >>> - else >>> - error = ret; >>> + save_error = get_ucode_from_buffer_amd( >>> + mc_amd, buf, bufsize, &applied_offset); >>> + >>> + if ( save_error ) >>> + error = save_error >>> } >>> - if ( !applied_offset || error ) >>> + if ( save_error ) >>> { >>> xfree(mc_amd); >>> uci->mc.mc_amd = mc_old; >>> } >>> + else >>> + xfree(mc_old); >> Won't this free mc_old (which is where previous microcode lived) even >> if the new buffer didn't have any valid microcode (i.e. applied_offset >> is zero)? >> >> -boris > save_error starts off as 1, and only gets set to 0 on a successful > get_ucode_from_buffer_amd(), so we will only free mc_old in the case > that we have valid microcde in mc_amd. All other cases free mc_amd and > revert to using mc_old. > > ~Andrew Ah, I missed that save_error is initialized to 1. Acked-by: Boris Ostrovsky > >> >>> out: >>> svm_host_osvw_init();