[Skiboot] [PATCH] fsp/ipmi: Fix an illegal memory access

[Mahesh Jagannath Salgaonkar]

On 07/13/2015 06:38 PM, Kamalesh Babulal wrote:
> 
> 
> On 07/13/2015 05:32 PM, Neelesh Gupta wrote:
>> Hi Kamalesh,
>>
>> On 07/13/2015 02:59 PM, Kamalesh Babulal wrote:
>>> On 07/13/2015 10:36 AM, Neelesh Gupta wrote:
>>>> The patch fixes an illegal access to the memory which has been
>>>> freed.
>>>>
>>>> Fixes Coverity defect # 101858
>>>>
>>>> Signed-off-by: Neelesh Gupta <neelegup at linux.vnet.ibm.com>
>>>> ---
>>>>  hw/fsp/fsp-ipmi.c |    3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/fsp/fsp-ipmi.c b/hw/fsp/fsp-ipmi.c
>>>> index e5cd6e2..729ff93 100644
>>>> --- a/hw/fsp/fsp-ipmi.c
>>>> +++ b/hw/fsp/fsp-ipmi.c
>>>> @@ -87,13 +87,12 @@ static void fsp_ipmi_req_complete(struct fsp_msg *msg)
>>>>  {
>>>>  	uint8_t status = (msg->resp->word1 >> 8) & 0xff;
>>>>  	uint32_t length = msg->resp->data.words[0];
>>>> -	struct fsp_ipmi_msg *fsp_ipmi_msg;
>>>> +	struct fsp_ipmi_msg *fsp_ipmi_msg = msg->user_data;
>>>>  	struct ipmi_msg *ipmi_msg;
>>>>
>>>>  	fsp_freemsg(msg);
>>>>
>>>>  	if (status != FSP_STATUS_SUCCESS) {
>>>> -		fsp_ipmi_msg = msg->user_data;
>>>>  		assert(fsp_ipmi_msg == fsp_ipmi.cur_msg);
>>>>
>>>>  		ipmi_msg = &fsp_ipmi_msg->ipmi_msg;
>>> fsp_free(msg) means msg->user_data is also un-allocated. It means
>>> fsp_ipmi_msg is pointing
>>> to memory which is freed. From the code, I am guessing you need
>>> free(msg) before you
>>> call fsp_ipmi_send_request(). If my assumption is right, how about below
>>> code ?
>>
>> 'msg->user_data' is a private data to this driver and the driver has to
>> take care of it. For instance, 'fsp_ipmi_msg' can only be free'd through
>> a backend callback i.e., fsp_ipmi_free_msg() in this driver.
>>
> 
> Sorry let me re-phrase it better what I meant was, does the code tries
> to achieve:
> 
> |_ fsp_freemsg(msg)
>     |_ fsp_impi_send_request()
> 
> If so, it better to move fsp_freemsg(msg) right before calling
> fsp_impi_send_request(), as
> the data deferenced will be still pointing to valid memory.

As Neelesh mentioned in his previous reply, msg->user_data is a private
data to fsp-ipmi driver and it keeps track of it through
fsp_ipmi.msg_queue and fsp_ipmi.cur_msg. Hence even if you free msg
using fsp_freemsg() the valid memory (pointed by msg->user_data) is
still accessible through fsp_ipmi.cur_msg.

Thanks,
-Mahesh.

> 
>>
>>> --- a/hw/fsp/fsp-ipmi.c
>>> +++ b/hw/fsp/fsp-ipmi.c
>>> @@ -90,8 +90,6 @@ static void fsp_ipmi_req_complete(struct fsp_msg *msg)
>>>         struct fsp_ipmi_msg *fsp_ipmi_msg;
>>>         struct ipmi_msg *ipmi_msg;
>>>
>>> -       fsp_freemsg(msg);
>>> -
>>>         if (status != FSP_STATUS_SUCCESS) {
>>>                 fsp_ipmi_msg = msg->user_data;
>>>                 assert(fsp_ipmi_msg == fsp_ipmi.cur_msg);
>>> @@ -111,9 +109,14 @@ static void fsp_ipmi_req_complete(struct fsp_msg *msg)
>>>                                   IPMI_NETFN_RETURN_CODE(ipmi_msg->netfn),
>>>                                   IPMI_ERR_UNSPECIFIED);
>>>
>>> +              fsp_freemsg(msg);
>>> +
>>>                 /* Send the next request in the queue */
>>>                 fsp_ipmi_send_request();
>>> +              return;
>>>         }
>>> +
>>> +       fsp_freemsg(msg);
>>>  }
>>>
>>>  static int fsp_ipmi_send_request(void)
> 
> 
>>
>