Re: [PATCH] RDMA/core: Insure security pkey modify is not lost

From: Jason Gunthorpe <jgg@ziepe.ca>
To: Mike Marciniszyn <mike.marciniszyn@intel.com>,
	Maor Gottlieb <maorg@mellanox.com>,
	Leon Romanovsky <leonro@mellanox.com>
Cc: dledford@redhat.com, linux-rdma@vger.kernel.org
Subject: Re: [PATCH] RDMA/core: Insure security pkey modify is not lost
Date: Wed, 18 Mar 2020 10:31:10 -0300	[thread overview]
Message-ID: <20200318133110.GA25617@ziepe.ca> (raw)
In-Reply-To: <20200313124704.14982.55907.stgit@awfm-01.aw.intel.com>

On Fri, Mar 13, 2020 at 08:47:05AM -0400, Mike Marciniszyn wrote:
> The following modify sequence (loosely based on ipoib) will
> lose a pkey modifcation:
> 
> - Modify (pkey index, port)
> - Modify (new pkey index, NO port)
> 
> After the first modify, the qp_pps list will have saved the pkey and the
> unit on the main list.
> 
> During the second modify, get_new_pps() will fetch the port from qp_pps
> and read the new pkey index from qp_attr->pkey_index.  The state will
> still be zero, or IB_PORT_PKEY_NOT_VALID. Because of the invalid state,
> the new values will never replace the one in the qp pps list, losing
> the new pkey.
> 
> This happens because the following if statements will never correct the
> state because the first term will be false. If the code had been executed,
> it would incorrectly overwrite valid values.
> 
> if ((qp_attr_mask & IB_QP_PKEY_INDEX) && (qp_attr_mask & IB_QP_PORT))
> 	new_pps->main.state = IB_PORT_PKEY_VALID;
> 
> 
> if (!(qp_attr_mask & (IB_QP_PKEY_INDEX | IB_QP_PORT)) && qp_pps) {
> 	new_pps->main.port_num = qp_pps->main.port_num;
> 	new_pps->main.pkey_index = qp_pps->main.pkey_index;
> 	if (qp_pps->main.state != IB_PORT_PKEY_NOT_VALID)
> 		new_pps->main.state = IB_PORT_PKEY_VALID;
> }
> 
> Fix by joining the two if statements with an or test to see if qp_pps
> is non-NULL and in the correct state.
> 
> Fixes: 1dd017882e01 ("RDMA/core: Fix protection fault in get_pkey_idx_qp_list")
> Reviewed-by: Kaike Wan <kaike.wan@intel.com>
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com>

Leon? Maor?

> diff --git a/drivers/infiniband/core/security.c b/drivers/infiniband/core/security.c
> index 2d56083..75e7ec0 100644
> +++ b/drivers/infiniband/core/security.c
> @@ -349,16 +349,11 @@ static struct ib_ports_pkeys *get_new_pps(const struct ib_qp *qp,
>  	else if (qp_pps)
>  		new_pps->main.pkey_index = qp_pps->main.pkey_index;
>  
> -	if ((qp_attr_mask & IB_QP_PKEY_INDEX) && (qp_attr_mask & IB_QP_PORT))
> +	if (((qp_attr_mask & IB_QP_PKEY_INDEX) &&
> +	     (qp_attr_mask & IB_QP_PORT)) ||
> +	    (qp_pps && qp_pps->main.state != IB_PORT_PKEY_NOT_VALID))
>  		new_pps->main.state = IB_PORT_PKEY_VALID;
>  
> -	if (!(qp_attr_mask & (IB_QP_PKEY_INDEX | IB_QP_PORT)) && qp_pps) {
> -		new_pps->main.port_num = qp_pps->main.port_num;
> -		new_pps->main.pkey_index = qp_pps->main.pkey_index;
> -		if (qp_pps->main.state != IB_PORT_PKEY_NOT_VALID)
> -			new_pps->main.state = IB_PORT_PKEY_VALID;
> -	}
> -
>  	if (qp_attr_mask & IB_QP_ALT_PATH) {
>  		new_pps->alt.port_num = qp_attr->alt_port_num;
>  		new_pps->alt.pkey_index = qp_attr->alt_pkey_index;
> 