Add NULL Check to allow IpmiSendCommandInternal flow through avoinding assert as routine does not consider NULL para…

[anandakrishnanl]

Description

Add NULL Check as IpmiSendCommandInternal does not consider NULL parameter for ResponseData and ResponseDataSize Parameter

Current Behavior:

IpmiSendCommandInternal does not consider NULL parameter for ResponseData and ResponseDataSize Parameter

Expected Behavior:

NULL can be passed as parameter for

IN OUT UINT8 *ResponseData,
IN OUT UINT8 *ResponseDataSize

in IpmiSendCommandInternal Routine

FIX APPLIED:

Add NULL Check as IpmiSendCommandInternal does not consider NULL parameter for ResponseData and ResponseDataSize Parameter

• Impacts functionality?
  • Functionality - Does the change ultimately impact how firmware functions?
  • Examples: Add a new library, publish a new PPI, update an algorithm, ...
• Impacts security?
  • Security - Does the change have a direct security impact on an application,
    flow, or firmware?
  • Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
• Breaking change?
  • Breaking change - Will anyone consuming this change experience a break
    in build or boot behavior?
  • Examples: Add a new library class, move a module to a different repo, call
    a function in a new library class in a pre-existing module, ...
• Includes tests?
  • Tests - Does the change include any explicit test code?
  • Examples: Unit tests, integration tests, robot tests, ...
• Includes documentation?
  • Documentation - Does the change contain explicit documentation additions
    outside direct code modifications (and comments)?
  • Examples: Update readme file, add feature readme file, link to documentation
    on an a separate Web page, ...

How This Was Tested

IpmiRasStartToDisableBmcRas command was issued with NULL parameter for ResponseData and ResponseDataSIze.
WithoutFIx, cpu exception is observed and with Fix, IPMI send Command can proceed.

Integration Instructions

N/A

[cfernald]

Could you update the title to indicate this is allowing for a null instance of the response on a send? Right now its unclear if you are adding a check to prevent or allow this.

[cfernald]

I would have a check at the beginning that will return invalid parameter if (ResponseDataSize == NULL) != (ResponseData)

Then you can just put this whole block under a (ResponseData != NULL) check instead of two different checks.

[anandakrishnanl]

Checking at the beginning and return Invalid parameter will return right at the start of the routine.

There are some valid commands that can have NULL for ResponseDataSize & Response Data but still want to send command to BMC.

[cfernald]

Sure, but my point is that a caller should never pass NULL for one of those but not the other. Adding that check before doing any work would have clarify the behavior of this functionas well as allow you to clean up and centralize these checks. specifically, (ResponseDataSize == NULL) != (ResponseData == NULL) (which i realize i didn't write correctly the first time)

[anandakrishnanl]

Hi Chris,

I have made changes in function header routine in both c & .h files as below:

IN OUT UINT8 *ResponseData OPTIONAL,
IN OUT UINT8 *ResponseDataSize OPTIONAL

For your comment for the check, still I am unsure why it required to be at the early and a centralised check.

Even though these 2 passed arguments are a pointer of a data and its size, they both are 2 different entities passed as pointer for the routine. Being an optional parameter, the check is being made only when is being used and is not interdependent on each other. That is the way code has been modified.

By this way it takes care of cases: Both parameters are NULL, Both parameters are NOT NULL and one of the parameter is NULL.

Early check and breaking with a return would still will not execute the required code in NULL passed with intentionally required,

Do clarify me on if I am not missing any case that you are foreseeing.

[cfernald]

Need to update the header here and in the .h file to add the OPTIONAL note for ResponseData and ResponseDataSize

[anandakrishnanl]

Could you update the title to indicate this is allowing for a null instance of the response on a send? Right now its unclear if you are adding a check to prevent or allow this.

Modified the Title

[anandakrishnanl]

@anandakrishnanl please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

• (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.

@microsoft-github-policy-service agree

• (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="AMI"