Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 25 May 2020 21:31:10 -0300
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Divya Indi <divya.indi@oracle.com>
Cc:     linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
        Kaike Wan <kaike.wan@intel.com>,
        Gerd Rausch <gerd.rausch@oracle.com>,
        =?utf-8?B?SMOla29u?= Bugge <haakon.bugge@oracle.com>,
        Srinivas Eeda <srinivas.eeda@oracle.com>,
        Rama Nichanamatlu <rama.nichanamatlu@oracle.com>,
        Doug Ledford <dledford@redhat.com>
Subject: Re: [PATCH] IB/sa: Resolving use-after-free in ib_nl_send_msg.
Message-ID: <20200526003110.GJ744@ziepe.ca>
References: <1589469084-15125-1-git-send-email-divya.indi@oracle.com>
 <1589469084-15125-2-git-send-email-divya.indi@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1589469084-15125-2-git-send-email-divya.indi@oracle.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 14, 2020 at 08:11:24AM -0700, Divya Indi wrote:
>  static void ib_nl_set_path_rec_attrs(struct sk_buff *skb,
>  				     struct ib_sa_query *query)
>  {
> @@ -889,6 +904,15 @@ static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
>  		spin_lock_irqsave(&ib_nl_request_lock, flags);
>  		list_del(&query->list);
>  		spin_unlock_irqrestore(&ib_nl_request_lock, flags);
> +	} else {
> +		set_bit(IB_SA_NL_QUERY_SENT, (unsigned long *)&query->flags);
> +
> +		/*
> +		 * If response is received before this flag was set
> +		 * someone is waiting to process the response and release the
> +		 * query.
> +		 */
> +		wake_up(&wait_queue);
>  	}

As far as I can see the issue here is that the request is put into the
ib_nl_request_list before it is really ready to be in that list, eg
ib_nl_send_msg() has actually completed and ownership of the memory
has been transfered.

It appears to me the reason for this is simply because a spinlock is
used for the ib_nl_request_lock and it cannot be held across
ib_nl_send_msg().

Convert that lock to a mutex and move the list_add to after the
success of ib_nl_send_msg() and this bug should be fixed without
adding jaunty atomics or a wait queue.

This is a 'racy error unwind' bug class...

Jason