From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by mx.groups.io with SMTP id smtpd.web09.10056.1614253078458246002 for ; Thu, 25 Feb 2021 03:37:58 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=STpn8GSP; spf=pass (domain: nuviainc.com, ip: 209.85.128.48, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f48.google.com with SMTP id x16so4371897wmk.3 for ; Thu, 25 Feb 2021 03:37:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=dukriG549xaNJgNxDxx+1ytUQZz7Rp5VIkuWe5JUMSQ=; b=STpn8GSP4XKUvLDMgnJKufrS3D6EuwZ2lyNP5w8Xq/JqMKW0xBadVyVmik8ukpDak8 IHTBMLv2TfQ6m0ORYCdVDF7mxnYtiDkZkBONWpSSkL8HgJ/GIqSdg8hM4fUBnAXWKSRn H6ZC0lo8wmFknywSbUxrKA64VllblUTMU8BRKatJw8z8k3ZohbyBCQWvX44r/QLTXYb+ wgnf0wN46N5VHScV1nWTXkBzp8N+DDgUPoV5o1v4oF7cxRUoVqC7sYWn25pJt3yf6Eyb 7KCqVmeTaOw5NpWshcaAkbzyaBF6qP8oS0yQ9RraMYMfwcsGDWxAYQtZa1jK3vrfycfx Bscg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=dukriG549xaNJgNxDxx+1ytUQZz7Rp5VIkuWe5JUMSQ=; b=YgJi7GdlhAucr0ZvI9t0yA/xZfIxBQ6XTvd3Y5Li/LDgtVzOnXgaEPfACTrIEX9Mw9 ZBbfk5Q3dzjcOkVO2Z2aaP4/grEzdbwPqS2jsTa0oYe5fFOZO9HrJ6Ks14LOUMlMnQFt xxmwQ7cjPRonJmGdwIg5IBEfRhXuFDF64Y54zjmGZDdCFw2ws+m822PpGJ00ABF02kAh z2t+X7fB3aRsWeWHHNnNdlD4bSUMEC7mw/qy+Nksr6Rc/HJLZWXs9y0bqjdmExcCkKK2 c7Oe2jswOXnPAOcIzZpzKRIxcjE801435pbvF65Dz6bqt3Z9SlTgdIuBEcCRRjo8Yc6A m2dg== X-Gm-Message-State: AOAM532/opbzWDIBdKEA8BqBnnB4lWRUQ4nh0jUu+cYmiDWVN4JZ1eYP sll8vllAZrGbrAPB41xRK+DN2w== X-Google-Smtp-Source: ABdhPJz15f8RlLap3MDb0J51lNFn61bbCiT0Nte9837Hm2skYvHtTE/CEFXajuBcWZRHUXyuAyi8UA== X-Received: by 2002:a1c:ac86:: with SMTP id v128mr2807375wme.175.1614253071978; Thu, 25 Feb 2021 03:37:51 -0800 (PST) Return-Path: Received: from vanye (cpc1-cmbg19-2-0-cust915.5-4.cable.virginm.net. [82.27.183.148]) by smtp.gmail.com with ESMTPSA id o13sm9887147wro.15.2021.02.25.03.37.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Feb 2021 03:37:51 -0800 (PST) Date: Thu, 25 Feb 2021 11:37:49 +0000 From: "Leif Lindholm" To: Sami Mujawar Cc: devel@edk2.groups.io, ardb+tianocore@kernel.org, Matteo.Carlini@arm.com, Ben.Adderson@arm.com, nd@arm.com Subject: Re: [PATCH v1 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib Message-ID: <20210225113749.GO1664@vanye> References: <20210224193756.24132-1-sami.mujawar@arm.com> MIME-Version: 1.0 In-Reply-To: <20210224193756.24132-1-sami.mujawar@arm.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Sami, On Wed, Feb 24, 2021 at 19:37:56 +0000, Sami Mujawar wrote: > The following patches added support for StandaloneMM using FF-A: > 9da5ee116a28 ArmPkg: Allow FF-A calls to set memory region's attributes > 0e43e02b9bd8 ArmPkg: Allow FF-A calls to get memory region's attributes > > However, the error handling logic for the Get/Set Memory attributes > introduced an issue wherein a status variable could be used without > initialisation. This issue is reported by CLANG compiler and is not > seen with GCC. > > The Get/Set Memory attributes operation is atomic and therefore an > FFA_INTERRUPT or FFA_SUCCESS response is not expected in response > to FFA_MSG_SEND_DIRECT_REQ. So the remaining cases that could occur > are: > - the target sends FFA_MSG_SEND_DIRECT_RESP with a success or > failure code. > or > - FFA_MSG_SEND_DIRECT_REQ transmission failure. > > Therefore, reorder the error handling conditions such that the > uninitialised variable issue is fixed. > > Signed-off-by: Sami Mujawar > --- > The changes can be seen at: > https://github.com/samimujawar/edk2/tree/1657_stmm_ffa_fix_unused_var_v1 > > ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c | 92 ++++++++++---------- > 1 file changed, 45 insertions(+), 47 deletions(-) > > diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c > index a30369af9c91fb8045dfec7a68e2bd072706d101..73b63ca396e5395bdf2112709b0aa2ab871a2a07 100644 > --- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c > +++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c > @@ -57,36 +57,35 @@ GetMemoryPermissions ( > // for other Direct Request calls which are not atomic > // We therefore check only for Direct Response by the > // callee. > - if (GetMemoryPermissionsSvcArgs.Arg0 != > + if (GetMemoryPermissionsSvcArgs.Arg0 == > ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) { > - // If Arg0 is not a Direct Response, that means we > - // have an FF-A error. We need to check Arg2 for the > - // FF-A error code. > - Ret = GetMemoryPermissionsSvcArgs.Arg2; > - switch (Ret) { > - case ARM_FFA_SPM_RET_INVALID_PARAMETERS: > - > - return EFI_INVALID_PARAMETER; > - > - case ARM_FFA_SPM_RET_DENIED: > - return EFI_NOT_READY; > - > - case ARM_FFA_SPM_RET_NOT_SUPPORTED: > - return EFI_UNSUPPORTED; > - > - case ARM_FFA_SPM_RET_BUSY: > - return EFI_NOT_READY; > - > - case ARM_FFA_SPM_RET_ABORTED: > - return EFI_ABORTED; > - } > - } else if (GetMemoryPermissionsSvcArgs.Arg0 == > - ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) { > // A Direct Response means FF-A success > // Now check the payload for errors > // The callee sends back the return value > // in Arg3 > Ret = GetMemoryPermissionsSvcArgs.Arg3; > + } else { > + // If Arg0 is not a Direct Response, that means we > + // have an FF-A error. We need to check Arg2 for the > + // FF-A error code. > + Ret = GetMemoryPermissionsSvcArgs.Arg2; > + switch (Ret) { > + case ARM_FFA_SPM_RET_INVALID_PARAMETERS: > + > + return EFI_INVALID_PARAMETER; > + > + case ARM_FFA_SPM_RET_DENIED: > + return EFI_NOT_READY; > + > + case ARM_FFA_SPM_RET_NOT_SUPPORTED: > + return EFI_UNSUPPORTED; > + > + case ARM_FFA_SPM_RET_BUSY: > + return EFI_NOT_READY; > + > + case ARM_FFA_SPM_RET_ABORTED: > + return EFI_ABORTED; > + } > } > } else { > Ret = GetMemoryPermissionsSvcArgs.Arg0; > @@ -150,35 +149,34 @@ RequestMemoryPermissionChange ( > // for other Direct Request calls which are not atomic > // We therefore check only for Direct Response by the > // callee. > - if (ChangeMemoryPermissionsSvcArgs.Arg0 != > + if (ChangeMemoryPermissionsSvcArgs.Arg0 == > ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) { > - // If Arg0 is not a Direct Response, that means we > - // have an FF-A error. We need to check Arg2 for the > - // FF-A error code. > - Ret = ChangeMemoryPermissionsSvcArgs.Arg2; > - switch (Ret) { > - case ARM_FFA_SPM_RET_INVALID_PARAMETERS: > - return EFI_INVALID_PARAMETER; > - > - case ARM_FFA_SPM_RET_DENIED: > - return EFI_NOT_READY; > - > - case ARM_FFA_SPM_RET_NOT_SUPPORTED: > - return EFI_UNSUPPORTED; > - > - case ARM_FFA_SPM_RET_BUSY: > - return EFI_NOT_READY; > - > - case ARM_FFA_SPM_RET_ABORTED: > - return EFI_ABORTED; > - } > - } else if (ChangeMemoryPermissionsSvcArgs.Arg0 == > - ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) { > // A Direct Response means FF-A success > // Now check the payload for errors > // The callee sends back the return value > // in Arg3 > Ret = ChangeMemoryPermissionsSvcArgs.Arg3; > + } else { > + // If Arg0 is not a Direct Response, that means we > + // have an FF-A error. We need to check Arg2 for the > + // FF-A error code. > + Ret = ChangeMemoryPermissionsSvcArgs.Arg2; > + switch (Ret) { > + case ARM_FFA_SPM_RET_INVALID_PARAMETERS: > + return EFI_INVALID_PARAMETER; > + > + case ARM_FFA_SPM_RET_DENIED: > + return EFI_NOT_READY; > + > + case ARM_FFA_SPM_RET_NOT_SUPPORTED: > + return EFI_UNSUPPORTED; > + > + case ARM_FFA_SPM_RET_BUSY: > + return EFI_NOT_READY; > + > + case ARM_FFA_SPM_RET_ABORTED: > + return EFI_ABORTED; > + } This patch applies the same change twice in the same file. It looks to me like the switch statement should be in a static helper function. This would also improve readability of both host functions. / Leif > } > } else { > Ret = ChangeMemoryPermissionsSvcArgs.Arg0; > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' >