Kloxwork Issue : NULL check after EVP_MD_CTX_new
EVP_MD_CTX_new could return NULL.
This Commit adds a NULL check to avoid NULL pointer dereferencing.
Tested:
By performing firmware update using following steps.
1. Generate fw update random number
ipmitool raw 0x08 0x26
2. Enter fw update mode
ipmitool raw 0x08 0x27 0xa8 0xa8 0x32 0x49 0x0a 0x28 0xef 0x40
3. Start Image transfer
ipmitool raw 0x08 0x29 0x01
4. Set Firmware update options
ipmitool raw 0x08 0x2B 0x04 0x04 0xe3 0xb0 0xc4 0x42 0x98 0xfc 0x1c
0x14 0x9a 0xfb 0xf4 0xc8 0x99 0x6f 0xb9 0x24 0x27 0xae 0x41 0xe4 0x64
0x9b 0x93 0x4c 0xa4 0x95 0x99 0x1b 0x78 0x52 0xb8 0x55
5. Set file name (file:///tmp/BMC.Bin)
ipmitool raw 0x08 0x29 0x04 0x13 0x66 0x69 0x6c 0x65 0x3a 0x2f 0x2f
0x2f 0x74 0x6d 0x70 0x2f 0x42 0x4d 0x43 0x2e 0x42 0x69 0x6e
6. End Image transfer
ipmitool raw 0x08 0x29 0x02
7. Exit fw-update mode
ipmitool raw 0x08 0x28
Signed-off-by: Arun Lal K M <arun.lal@intel.com>
Change-Id: Iefb83247278bfc58af209dad252f01c4bf01f2f4
diff --git a/src/firmware-update.cpp b/src/firmware-update.cpp
index 809b87a..1cd1161 100644
--- a/src/firmware-update.cpp
+++ b/src/firmware-update.cpp
@@ -34,6 +34,7 @@
#include <spiDev.hpp>
#endif
+static constexpr int openSslSuccess = 1;
static constexpr bool DEBUG = true;
static void registerFirmwareFunctions() __attribute__((constructor));
@@ -190,61 +191,64 @@
};
protected:
- EVP_MD_CTX* ctx;
+ std::unique_ptr<EVP_MD_CTX, std::function<void(EVP_MD_CTX*)>> ctx;
std::vector<uint8_t> expectedHash;
- enum HashCheck check;
- bool started;
+ HashCheck check;
public:
- TransferHashCheck() : check(HashCheck::notRequested), started(false)
- {}
- ~TransferHashCheck()
+ TransferHashCheck(const std::vector<uint8_t>& expected) :
+ ctx(EVP_MD_CTX_new(), &EVP_MD_CTX_free), expectedHash(expected)
{
- if (ctx)
+ if (!ctx)
{
- EVP_MD_CTX_destroy(ctx);
- ctx = NULL;
+ throw std::runtime_error("Unable to allocate for ctx.");
}
- }
- void init(const std::vector<uint8_t>& expected)
- {
- expectedHash = expected;
+
+ if (EVP_DigestInit(ctx.get(), EVP_sha256()) != openSslSuccess)
+ {
+ throw std::runtime_error("Unable to allocate for ctx.");
+ }
+
check = HashCheck::requested;
- ctx = EVP_MD_CTX_create();
- EVP_DigestInit(ctx, EVP_sha256());
}
- void hash(const std::vector<uint8_t>& data)
+
+ ~TransferHashCheck()
+ {}
+
+ bool hash(const std::vector<uint8_t>& data)
{
- if (!started)
+ if (EVP_DigestUpdate(ctx.get(), data.data(), data.size()) !=
+ openSslSuccess)
{
- started = true;
+ return false;
}
- EVP_DigestUpdate(ctx, data.data(), data.size());
+
+ return true;
}
- void clear()
+
+ bool clear()
{
- // if not started, nothing to clear
- if (started)
+ /*
+ * EVP_DigestInit() always uses the default digest implementation and
+ * calls EVP_MD_CTX_reset().
+ */
+ if (EVP_DigestInit(ctx.get(), EVP_sha256()) != openSslSuccess)
{
- if (ctx)
- {
- EVP_MD_CTX_destroy(ctx);
- }
- if (check != HashCheck::notRequested)
- {
- check = HashCheck::requested;
- }
- ctx = EVP_MD_CTX_create();
- EVP_DigestInit(ctx, EVP_sha256());
+ return false;
}
+
+ return true;
}
+
enum HashCheck verify()
{
- if (check == HashCheck::requested)
+ unsigned int len = 0;
+ std::vector<uint8_t> digest(EVP_MD_size(EVP_sha256()));
+
+ check = HashCheck::sha2Failed;
+
+ if (EVP_DigestFinal(ctx.get(), digest.data(), &len) == openSslSuccess)
{
- unsigned int len = 0;
- std::vector<uint8_t> digest(EVP_MD_size(EVP_sha256()));
- EVP_DigestFinal(ctx, digest.data(), &len);
if (digest == expectedHash)
{
phosphor::logging::log<phosphor::logging::level::INFO>(
@@ -258,8 +262,10 @@
check = HashCheck::sha2Failed;
}
}
+
return check;
}
+
uint8_t status() const
{
return static_cast<uint8_t>(check);
@@ -447,7 +453,7 @@
};
static FwUpdateStatusCache fwUpdateStatus;
-std::shared_ptr<TransferHashCheck> xferHashCheck;
+std::unique_ptr<TransferHashCheck> xferHashCheck;
static void activateImage(const std::string& objPath)
{
@@ -652,7 +658,7 @@
return true;
}
-static int transferImageFromFile(const std::string& uri, bool move = true)
+static bool transferImageFromFile(const std::string& uri, bool move = true)
{
std::error_code ec;
phosphor::logging::log<phosphor::logging::level::INFO>(
@@ -671,15 +677,22 @@
if (xferHashCheck)
{
MappedFile mappedfw(uri);
- xferHashCheck->hash(
- {mappedfw.data(), mappedfw.data() + mappedfw.size()});
+ if (!xferHashCheck->hash(
+ {mappedfw.data(), mappedfw.data() + mappedfw.size()}))
+ {
+ phosphor::logging::log<phosphor::logging::level::ERR>(
+ "transferImageFromFile: xferHashCheck->hash failed.");
+ return false;
+ }
}
if (ec.value())
{
phosphor::logging::log<phosphor::logging::level::ERR>(
"Image copy failed.");
+ return false;
}
- return ec.value();
+
+ return true;
}
template <typename... ArgTypes>
@@ -690,24 +703,25 @@
return execProg.exit_code();
}
-static int transferImageFromUsb(const std::string& uri)
+static bool transferImageFromUsb(const std::string& uri)
{
- int ret, sysret;
+ bool ret = false;
char fwpath[fwPathMaxLength];
+
phosphor::logging::log<phosphor::logging::level::INFO>(
"Transfer Image From USB.",
phosphor::logging::entry("URI=%s", uri.c_str()));
- ret = executeCmd(usbCtrlPath, "mount", fwUpdateUsbVolImage,
- fwUpdateMountPoint);
- if (ret)
+
+ if (executeCmd(usbCtrlPath, "mount", fwUpdateUsbVolImage,
+ fwUpdateMountPoint) == 0)
{
- return ret;
+ std::string usb_path = std::string(fwUpdateMountPoint) + "/" + uri;
+ ret = transferImageFromFile(usb_path, false);
+
+ executeCmd(usbCtrlPath, "cleanup", fwUpdateUsbVolImage,
+ fwUpdateMountPoint);
}
- std::string usb_path = std::string(fwUpdateMountPoint) + "/" + uri;
- ret = transferImageFromFile(usb_path, false);
-
- executeCmd(usbCtrlPath, "cleanup", fwUpdateUsbVolImage, fwUpdateMountPoint);
return ret;
}
@@ -723,14 +737,14 @@
std::string fname = uri.substr(sizeof(fwUriFile) - 1);
if (fname != firmwareBufferFile)
{
- return 0 == transferImageFromFile(fname);
+ return transferImageFromFile(fname);
}
return true;
}
if (boost::algorithm::starts_with(uri, fwUriUsb))
{
std::string fname = uri.substr(sizeof(fwUriUsb) - 1);
- return 0 == transferImageFromUsb(fname);
+ return transferImageFromUsb(fname);
}
return false;
}
@@ -1326,7 +1340,12 @@
fwXferUriPath = std::string("file://") + firmwareBufferFile;
if (xferHashCheck)
{
- xferHashCheck->clear();
+ if (!xferHashCheck->clear())
+ {
+ phosphor::logging::log<phosphor::logging::level::ERR>(
+ "clear() for xferHashCheck failed");
+ return ipmi::responseUnspecifiedError();
+ }
}
// Setting state to download
fwUpdateStatus.setState(
@@ -1525,8 +1544,18 @@
"Invalid size of Hash specified.");
return ipmi::responseInvalidFieldRequest();
}
- xferHashCheck = std::make_shared<TransferHashCheck>();
- xferHashCheck->init(*integrityCheckVal);
+
+ try
+ {
+ xferHashCheck =
+ std::make_unique<TransferHashCheck>(*integrityCheckVal);
+ }
+ catch (std::exception& ex)
+ {
+ phosphor::logging::log<phosphor::logging::level::ERR>(
+ ex.what());
+ return ipmi::responseUnspecifiedError();
+ }
}
else
{
@@ -1581,7 +1610,12 @@
if (xferHashCheck)
{
- xferHashCheck->hash(writeData);
+ if (!xferHashCheck->hash(writeData))
+ {
+ phosphor::logging::log<phosphor::logging::level::ERR>(
+ "ipmiFwImageWriteData: xferHashCheck->hash failed.");
+ return ipmi::responseUnspecifiedError();
+ }
}
#ifdef INTEL_PFR_ENABLED