[Skiboot] [PATCH] hw/bt.c: Fix message response search

[Benjamin Herrenschmidt]

On Wed, 2015-06-17 at 22:12 +1000, Alistair Popple wrote:
> bt_get_resp() uses the sequence number to locate the outstanding
> message by searching the queue with list_for_each(...). The check to
> see if a message was found in the queue is:
> 
>     if (!bt_msg || (bt_msg->seq != seq))

Ah, you found it !!! Don't forget to also fix the other one we found
yesterday :-)

> However this check is incorrect. list_for_each(...) does not set
> bt_msg to NULL at the end of the loop, nor does it set it to the last
> element in the list. Rather it ends up pointing at an offset from the
> list_head. Therefore bt_msg is never equal to NULL and the first half
> of this condition is always false.
> 
> However the second condition is almost always true as sequence numbers
> are constantly increasing and unlikely to match whatever bt_msg->seq
> ends up pointing to, but in rare circumstances it is possible for
> bt_msg->seq to equal seq and hence the overall expression can evaluate
> to false.
> 
> This leads to bt_msg being used as a valid message pointer even though
> it points to an incorrect address, typically causing a xstop due to an
> invalid address access when ipmi_cmd_done(...) attempts to call the
> callbacks.
> 
> Signed-off-by: Alistair Popple <alistair at popple.id.au>
> ---
>  hw/bt.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/bt.c b/hw/bt.c
> index 8a9e935..da59b14 100644
> --- a/hw/bt.c
> +++ b/hw/bt.c
> @@ -207,7 +207,7 @@ static void bt_flush_msg(void)
>  static void bt_get_resp(void)
>  {
>  	int i;
> -	struct bt_msg *bt_msg;
> +	struct bt_msg *tmp_bt_msg, *bt_msg = NULL;
>  	struct ipmi_msg *ipmi_msg;
>  	uint8_t resp_len, netfn, seq, cmd;
>  	uint8_t cc = IPMI_CC_NO_ERROR;
> @@ -236,13 +236,14 @@ static void bt_get_resp(void)
>  	cc = bt_inb(BT_HOST2BMC);
>  
>  	/* Find the corresponding messasge */
> -	list_for_each(&bt.msgq, bt_msg, link) {
> -		if (bt_msg->seq == seq) {
> +	list_for_each(&bt.msgq, tmp_bt_msg, link) {
> +		if (tmp_bt_msg->seq == seq) {
> +			bt_msg = tmp_bt_msg;
>  			break;
>  		}
>  
>  	}
> -	if (!bt_msg || (bt_msg->seq != seq)) {
> +	if (!bt_msg) {
>  		/* A response to a message we no longer care about. */
>  		prlog(PR_INFO, "BT: Nobody cared about a response to an BT/IPMI message\n");
>  		bt_flush_msg();