[PATCH] RDMA/mana_ib: Prevent array underflow in mana_ib_create_qp_raw()

[PATCH] RDMA/mana_ib: Prevent array underflow in mana_ib_create_qp_raw()

[Dan Carpenter]

The "port" comes from the user and if it is zero then the:

	ndev = mc->ports[port - 1];

assignment does an out of bounds read.  I have changed the if
statement to fix this and to mirror how it is done in
mana_ib_create_qp_rss().

Fixes: 0266a177631d ("RDMA/mana_ib: Add a driver for Microsoft Azure Network Adapter")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
 drivers/infiniband/hw/mana/qp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
index ea15ec77e321..54b61930a7fd 100644
--- a/drivers/infiniband/hw/mana/qp.c
+++ b/drivers/infiniband/hw/mana/qp.c
@@ -289,7 +289,7 @@ static int mana_ib_create_qp_raw(struct ib_qp *ibqp, struct ib_pd *ibpd,
 
 	/* IB ports start with 1, MANA Ethernet ports start with 0 */
 	port = ucmd.port;
-	if (ucmd.port > mc->num_ports)
+	if (port < 1 || port > mc->num_ports)
 		return -EINVAL;
 
 	if (attr->cap.max_send_wr > MAX_SEND_BUFFERS_PER_QUEUE) {
-- 
2.35.1

RE: [PATCH] RDMA/mana_ib: Prevent array underflow in mana_ib_create_qp_raw()

[Long Li]

> Subject: [PATCH] RDMA/mana_ib: Prevent array underflow in
> mana_ib_create_qp_raw()
> 
> The "port" comes from the user and if it is zero then the:
> 
> 	ndev = mc->ports[port - 1];
> 
> assignment does an out of bounds read.  I have changed the if statement to fix
> this and to mirror how it is done in mana_ib_create_qp_rss().
> 
> Fixes: 0266a177631d ("RDMA/mana_ib: Add a driver for Microsoft Azure
> Network Adapter")
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Thank you.

Acked-by: Long Li <longli@microsoft.com>

> ---
>  drivers/infiniband/hw/mana/qp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
> index ea15ec77e321..54b61930a7fd 100644
> --- a/drivers/infiniband/hw/mana/qp.c
> +++ b/drivers/infiniband/hw/mana/qp.c
> @@ -289,7 +289,7 @@ static int mana_ib_create_qp_raw(struct ib_qp *ibqp,
> struct ib_pd *ibpd,
> 
>  	/* IB ports start with 1, MANA Ethernet ports start with 0 */
>  	port = ucmd.port;
> -	if (ucmd.port > mc->num_ports)
> +	if (port < 1 || port > mc->num_ports)
>  		return -EINVAL;
> 
>  	if (attr->cap.max_send_wr > MAX_SEND_BUFFERS_PER_QUEUE) {
> --
> 2.35.1

Re: [PATCH] RDMA/mana_ib: Prevent array underflow in mana_ib_create_qp_raw()

[Leon Romanovsky]

On Tue, Jan 24, 2023 at 06:20:54PM +0300, Dan Carpenter wrote:
> The "port" comes from the user and if it is zero then the:
> 
> 	ndev = mc->ports[port - 1];
> 
> assignment does an out of bounds read.  I have changed the if
> statement to fix this and to mirror how it is done in
> mana_ib_create_qp_rss().
> 
> Fixes: 0266a177631d ("RDMA/mana_ib: Add a driver for Microsoft Azure Network Adapter")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
>  drivers/infiniband/hw/mana/qp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
> index ea15ec77e321..54b61930a7fd 100644
> --- a/drivers/infiniband/hw/mana/qp.c
> +++ b/drivers/infiniband/hw/mana/qp.c
> @@ -289,7 +289,7 @@ static int mana_ib_create_qp_raw(struct ib_qp *ibqp, struct ib_pd *ibpd,
>  
>  	/* IB ports start with 1, MANA Ethernet ports start with 0 */
>  	port = ucmd.port;
> -	if (ucmd.port > mc->num_ports)
> +	if (port < 1 || port > mc->num_ports)

Why do I see port in mana_ib_create_qp? It should come from ib_qp_init_attr.

Thanks


>  		return -EINVAL;
>  
>  	if (attr->cap.max_send_wr > MAX_SEND_BUFFERS_PER_QUEUE) {
> -- 
> 2.35.1
> 

Re: [PATCH] RDMA/mana_ib: Prevent array underflow in mana_ib_create_qp_raw()

[Dan Carpenter]

On Thu, Jan 26, 2023 at 12:18:46PM +0200, Leon Romanovsky wrote:
> On Tue, Jan 24, 2023 at 06:20:54PM +0300, Dan Carpenter wrote:
> > The "port" comes from the user and if it is zero then the:
> > 
> > 	ndev = mc->ports[port - 1];
> > 
> > assignment does an out of bounds read.  I have changed the if
> > statement to fix this and to mirror how it is done in
> > mana_ib_create_qp_rss().
> > 
> > Fixes: 0266a177631d ("RDMA/mana_ib: Add a driver for Microsoft Azure Network Adapter")
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > ---
> >  drivers/infiniband/hw/mana/qp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
> > index ea15ec77e321..54b61930a7fd 100644
> > --- a/drivers/infiniband/hw/mana/qp.c
> > +++ b/drivers/infiniband/hw/mana/qp.c
> > @@ -289,7 +289,7 @@ static int mana_ib_create_qp_raw(struct ib_qp *ibqp, struct ib_pd *ibpd,
> >  
> >  	/* IB ports start with 1, MANA Ethernet ports start with 0 */
> >  	port = ucmd.port;
> > -	if (ucmd.port > mc->num_ports)
> > +	if (port < 1 || port > mc->num_ports)
> 
> Why do I see port in mana_ib_create_qp? It should come from ib_qp_init_attr.

I am so confused by this question.  Are you asking me?  This is the _raw
function.  I'm now sure what mana_ib_create_qp() has to do with it.

The port comes from ib_copy_from_udata() which is just a wrapper around
copy_from_user().

regards,
dan carpenter

Re: [PATCH] RDMA/mana_ib: Prevent array underflow in mana_ib_create_qp_raw()

[Leon Romanovsky]

On Thu, Jan 26, 2023 at 02:00:45PM +0300, Dan Carpenter wrote:
> On Thu, Jan 26, 2023 at 12:18:46PM +0200, Leon Romanovsky wrote:
> > On Tue, Jan 24, 2023 at 06:20:54PM +0300, Dan Carpenter wrote:
> > > The "port" comes from the user and if it is zero then the:
> > > 
> > > 	ndev = mc->ports[port - 1];
> > > 
> > > assignment does an out of bounds read.  I have changed the if
> > > statement to fix this and to mirror how it is done in
> > > mana_ib_create_qp_rss().
> > > 
> > > Fixes: 0266a177631d ("RDMA/mana_ib: Add a driver for Microsoft Azure Network Adapter")
> > > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > > ---
> > >  drivers/infiniband/hw/mana/qp.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
> > > index ea15ec77e321..54b61930a7fd 100644
> > > --- a/drivers/infiniband/hw/mana/qp.c
> > > +++ b/drivers/infiniband/hw/mana/qp.c
> > > @@ -289,7 +289,7 @@ static int mana_ib_create_qp_raw(struct ib_qp *ibqp, struct ib_pd *ibpd,
> > >  
> > >  	/* IB ports start with 1, MANA Ethernet ports start with 0 */
> > >  	port = ucmd.port;
> > > -	if (ucmd.port > mc->num_ports)
> > > +	if (port < 1 || port > mc->num_ports)
> > 
> > Why do I see port in mana_ib_create_qp? It should come from ib_qp_init_attr.
> 
> I am so confused by this question.  Are you asking me?  

I asked *@microsoft folks.

> This is the _raw function. 

_raw comes from QP type, it is not raw (basic) in a sense you imagine.

> I'm now sure what mana_ib_create_qp() has to do with it.

All create QP calls come through same verbs interface.
ib_create_qp_user->create_qp->.create_qp->mana_ib_create_qp->mana_ib_create_qp_raw

> 
> The port comes from ib_copy_from_udata() which is just a wrapper around
> copy_from_user().

Right, and it shouldn't.

> 
> regards,
> dan carpenter
> 

RE: [PATCH] RDMA/mana_ib: Prevent array underflow in mana_ib_create_qp_raw()

[Long Li]

> Subject: Re: [PATCH] RDMA/mana_ib: Prevent array underflow in
> mana_ib_create_qp_raw()
> 
> On Thu, Jan 26, 2023 at 02:00:45PM +0300, Dan Carpenter wrote:
> > On Thu, Jan 26, 2023 at 12:18:46PM +0200, Leon Romanovsky wrote:
> > > On Tue, Jan 24, 2023 at 06:20:54PM +0300, Dan Carpenter wrote:
> > > > The "port" comes from the user and if it is zero then the:
> > > >
> > > > 	ndev = mc->ports[port - 1];
> > > >
> > > > assignment does an out of bounds read.  I have changed the if
> > > > statement to fix this and to mirror how it is done in
> > > > mana_ib_create_qp_rss().
> > > >
> > > > Fixes: 0266a177631d ("RDMA/mana_ib: Add a driver for Microsoft
> > > > Azure Network Adapter")
> > > > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > > > ---
> > > >  drivers/infiniband/hw/mana/qp.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/mana/qp.c
> > > > b/drivers/infiniband/hw/mana/qp.c index ea15ec77e321..54b61930a7fd
> > > > 100644
> > > > --- a/drivers/infiniband/hw/mana/qp.c
> > > > +++ b/drivers/infiniband/hw/mana/qp.c
> > > > @@ -289,7 +289,7 @@ static int mana_ib_create_qp_raw(struct ib_qp
> > > > *ibqp, struct ib_pd *ibpd,
> > > >
> > > >  	/* IB ports start with 1, MANA Ethernet ports start with 0 */
> > > >  	port = ucmd.port;
> > > > -	if (ucmd.port > mc->num_ports)
> > > > +	if (port < 1 || port > mc->num_ports)
> > >
> > > Why do I see port in mana_ib_create_qp? It should come from ib_qp_init_attr.
> >
> > I am so confused by this question.  Are you asking me?
> 
> I asked *@microsoft folks.
> 
> > This is the _raw function.
> 
> _raw comes from QP type, it is not raw (basic) in a sense you imagine.
> 
> > I'm now sure what mana_ib_create_qp() has to do with it.
> 
> All create QP calls come through same verbs interface.
> ib_create_qp_user->create_qp->.create_qp->mana_ib_create_qp-
> >mana_ib_create_qp_raw

MANA requires passing a port number when creating a RAW QP on a RDMA(Ethernet) port.
At the hardware layer, the RDMA port and ethernet port share the same hardware resources, 
the port number needs to be known in advance when QP is created. If we don't' specify the port,
the QP needs to take all the ports on the MANA device, some of them may have been assigned to
Ethernet usage and can't be used for RDMA.

The reason is that unlike Nvidia CX hardware, MANA doesn't support bifurcation (for RAW QP) at the hardware level.
[https://www.dpdk.org/wp-content/uploads/sites/35/2018/06/Mellanox-bifurcated-driver-model.pdf]
To support RAW QP on a hardware port, we need to know the port number before configuring it on the hardware.
And Ethernet can't use this port if a RAW QP is created on it. The coordination needs to be done in software.

In production environment, there are multiple ports on the same MANA device assigned to a VM. Customer can
configure some of the ports for Ethernet and some for RDMA/DPDK.

I have investigated using the port_num in struct ib_qp_init_attr, but it seems it can't be used for this purpose
because the port needs to be specified by the user-mode.

Thanks,
Long

> 
> >
> > The port comes from ib_copy_from_udata() which is just a wrapper
> > around copy_from_user().
> 
> Right, and it shouldn't.
> 
> >
> > regards,
> > dan carpenter
> >

Re: [PATCH] RDMA/mana_ib: Prevent array underflow in mana_ib_create_qp_raw()

[Leon Romanovsky]

On Fri, Jan 27, 2023 at 06:41:51AM +0000, Long Li wrote:
> > Subject: Re: [PATCH] RDMA/mana_ib: Prevent array underflow in
> > mana_ib_create_qp_raw()
> > 
> > On Thu, Jan 26, 2023 at 02:00:45PM +0300, Dan Carpenter wrote:
> > > On Thu, Jan 26, 2023 at 12:18:46PM +0200, Leon Romanovsky wrote:
> > > > On Tue, Jan 24, 2023 at 06:20:54PM +0300, Dan Carpenter wrote:
> > > > > The "port" comes from the user and if it is zero then the:
> > > > >
> > > > > 	ndev = mc->ports[port - 1];
> > > > >
> > > > > assignment does an out of bounds read.  I have changed the if
> > > > > statement to fix this and to mirror how it is done in
> > > > > mana_ib_create_qp_rss().
> > > > >
> > > > > Fixes: 0266a177631d ("RDMA/mana_ib: Add a driver for Microsoft
> > > > > Azure Network Adapter")
> > > > > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > > > > ---
> > > > >  drivers/infiniband/hw/mana/qp.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/hw/mana/qp.c
> > > > > b/drivers/infiniband/hw/mana/qp.c index ea15ec77e321..54b61930a7fd
> > > > > 100644
> > > > > --- a/drivers/infiniband/hw/mana/qp.c
> > > > > +++ b/drivers/infiniband/hw/mana/qp.c
> > > > > @@ -289,7 +289,7 @@ static int mana_ib_create_qp_raw(struct ib_qp
> > > > > *ibqp, struct ib_pd *ibpd,
> > > > >
> > > > >  	/* IB ports start with 1, MANA Ethernet ports start with 0 */
> > > > >  	port = ucmd.port;
> > > > > -	if (ucmd.port > mc->num_ports)
> > > > > +	if (port < 1 || port > mc->num_ports)
> > > >
> > > > Why do I see port in mana_ib_create_qp? It should come from ib_qp_init_attr.
> > >
> > > I am so confused by this question.  Are you asking me?
> > 
> > I asked *@microsoft folks.
> > 
> > > This is the _raw function.
> > 
> > _raw comes from QP type, it is not raw (basic) in a sense you imagine.
> > 
> > > I'm now sure what mana_ib_create_qp() has to do with it.
> > 
> > All create QP calls come through same verbs interface.
> > ib_create_qp_user->create_qp->.create_qp->mana_ib_create_qp-
> > >mana_ib_create_qp_raw
> 
> MANA requires passing a port number when creating a RAW QP on a RDMA(Ethernet) port.
> At the hardware layer, the RDMA port and ethernet port share the same hardware resources, 
> the port number needs to be known in advance when QP is created. If we don't' specify the port,
> the QP needs to take all the ports on the MANA device, some of them may have been assigned to
> Ethernet usage and can't be used for RDMA.
> 
> The reason is that unlike Nvidia CX hardware, MANA doesn't support bifurcation (for RAW QP) at the hardware level.
> [https://www.dpdk.org/wp-content/uploads/sites/35/2018/06/Mellanox-bifurcated-driver-model.pdf]
> To support RAW QP on a hardware port, we need to know the port number before configuring it on the hardware.
> And Ethernet can't use this port if a RAW QP is created on it. The coordination needs to be done in software.
> 
> In production environment, there are multiple ports on the same MANA device assigned to a VM. Customer can
> configure some of the ports for Ethernet and some for RDMA/DPDK.
> 
> I have investigated using the port_num in struct ib_qp_init_attr, but it seems it can't be used for this purpose
> because the port needs to be specified by the user-mode.

And this is the most problematic part for me. I don't think that it is
RDMA user responsibility to dig in netdev port numbers.

Jason, what do you think? It looks like layering violation.

Thanks

> 
> Thanks,
> Long
> 
> > 
> > >
> > > The port comes from ib_copy_from_udata() which is just a wrapper
> > > around copy_from_user().
> > 
> > Right, and it shouldn't.
> > 
> > >
> > > regards,
> > > dan carpenter
> > >

Re: [PATCH] RDMA/mana_ib: Prevent array underflow in mana_ib_create_qp_raw()

[Leon Romanovsky]

On Sun, Jan 29, 2023 at 01:27:07PM +0200, Leon Romanovsky wrote:
> On Fri, Jan 27, 2023 at 06:41:51AM +0000, Long Li wrote:
> > > Subject: Re: [PATCH] RDMA/mana_ib: Prevent array underflow in
> > > mana_ib_create_qp_raw()
> > > 
> > > On Thu, Jan 26, 2023 at 02:00:45PM +0300, Dan Carpenter wrote:
> > > > On Thu, Jan 26, 2023 at 12:18:46PM +0200, Leon Romanovsky wrote:
> > > > > On Tue, Jan 24, 2023 at 06:20:54PM +0300, Dan Carpenter wrote:
> > > > > > The "port" comes from the user and if it is zero then the:
> > > > > >
> > > > > > 	ndev = mc->ports[port - 1];
> > > > > >
> > > > > > assignment does an out of bounds read.  I have changed the if
> > > > > > statement to fix this and to mirror how it is done in
> > > > > > mana_ib_create_qp_rss().
> > > > > >
> > > > > > Fixes: 0266a177631d ("RDMA/mana_ib: Add a driver for Microsoft
> > > > > > Azure Network Adapter")
> > > > > > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > > > > > ---
> > > > > >  drivers/infiniband/hw/mana/qp.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/infiniband/hw/mana/qp.c
> > > > > > b/drivers/infiniband/hw/mana/qp.c index ea15ec77e321..54b61930a7fd
> > > > > > 100644
> > > > > > --- a/drivers/infiniband/hw/mana/qp.c
> > > > > > +++ b/drivers/infiniband/hw/mana/qp.c
> > > > > > @@ -289,7 +289,7 @@ static int mana_ib_create_qp_raw(struct ib_qp
> > > > > > *ibqp, struct ib_pd *ibpd,
> > > > > >
> > > > > >  	/* IB ports start with 1, MANA Ethernet ports start with 0 */
> > > > > >  	port = ucmd.port;
> > > > > > -	if (ucmd.port > mc->num_ports)
> > > > > > +	if (port < 1 || port > mc->num_ports)
> > > > >
> > > > > Why do I see port in mana_ib_create_qp? It should come from ib_qp_init_attr.
> > > >
> > > > I am so confused by this question.  Are you asking me?
> > > 
> > > I asked *@microsoft folks.
> > > 
> > > > This is the _raw function.
> > > 
> > > _raw comes from QP type, it is not raw (basic) in a sense you imagine.
> > > 
> > > > I'm now sure what mana_ib_create_qp() has to do with it.
> > > 
> > > All create QP calls come through same verbs interface.
> > > ib_create_qp_user->create_qp->.create_qp->mana_ib_create_qp-
> > > >mana_ib_create_qp_raw
> > 
> > MANA requires passing a port number when creating a RAW QP on a RDMA(Ethernet) port.
> > At the hardware layer, the RDMA port and ethernet port share the same hardware resources, 
> > the port number needs to be known in advance when QP is created. If we don't' specify the port,
> > the QP needs to take all the ports on the MANA device, some of them may have been assigned to
> > Ethernet usage and can't be used for RDMA.
> > 
> > The reason is that unlike Nvidia CX hardware, MANA doesn't support bifurcation (for RAW QP) at the hardware level.
> > [https://www.dpdk.org/wp-content/uploads/sites/35/2018/06/Mellanox-bifurcated-driver-model.pdf]
> > To support RAW QP on a hardware port, we need to know the port number before configuring it on the hardware.
> > And Ethernet can't use this port if a RAW QP is created on it. The coordination needs to be done in software.
> > 
> > In production environment, there are multiple ports on the same MANA device assigned to a VM. Customer can
> > configure some of the ports for Ethernet and some for RDMA/DPDK.
> > 
> > I have investigated using the port_num in struct ib_qp_init_attr, but it seems it can't be used for this purpose
> > because the port needs to be specified by the user-mode.
> 
> And this is the most problematic part for me. I don't think that it is
> RDMA user responsibility to dig in netdev port numbers.

ok, I don't like it, but bug is here, let's fix it.

> 
> Jason, what do you think? It looks like layering violation.
> 
> Thanks
> 
> > 
> > Thanks,
> > Long
> > 
> > > 
> > > >
> > > > The port comes from ib_copy_from_udata() which is just a wrapper
> > > > around copy_from_user().
> > > 
> > > Right, and it shouldn't.
> > > 
> > > >
> > > > regards,
> > > > dan carpenter
> > > >

Re: [PATCH] RDMA/mana_ib: Prevent array underflow in mana_ib_create_qp_raw()

[Leon Romanovsky]

On Tue, 24 Jan 2023 18:20:54 +0300, Dan Carpenter wrote:
> The "port" comes from the user and if it is zero then the:
> 
> 	ndev = mc->ports[port - 1];
> 
> assignment does an out of bounds read.  I have changed the if
> statement to fix this and to mirror how it is done in
> mana_ib_create_qp_rss().
> 
> [...]

Applied, thanks!

[1/1] RDMA/mana_ib: Prevent array underflow in mana_ib_create_qp_raw()
      https://git.kernel.org/rdma/rdma/c/29419daf6c957f

Best regards,
-- 
Leon Romanovsky <leon@kernel.org>