[PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

We have many WR opcodes that are only supported in kernel space
and/or require optional information to be copied into the WR
structure.  Reject all those not explicitly handled so that we
can't pass invalid information to drivers.

Cc: stable@vger.kernel.org
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/infiniband/core/uverbs_cmd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index a15318a..f9f3921 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2372,6 +2372,12 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 		next->send_flags = user_wr->send_flags;
 
 		if (is_ud) {
+			if (next->opcode != IB_WR_SEND &&
+			    next->opcode != IB_WR_SEND_WITH_IMM) {
+				ret = -EINVAL;
+				goto out_put;
+			}
+
 			next->wr.ud.ah = idr_read_ah(user_wr->wr.ud.ah,
 						     file->ucontext);
 			if (!next->wr.ud.ah) {
@@ -2413,7 +2419,8 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 				next->wr.atomic.rkey = user_wr->wr.atomic.rkey;
 				break;
 			default:
-				break;
+				ret = -EINVAL;
+				goto out_put;
 			}
 		}
 
-- 
1.9.1

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Jason Gunthorpe]

On Wed, Aug 19, 2015 at 06:37:32PM +0200, Christoph Hellwig wrote:
> We have many WR opcodes that are only supported in kernel space
> and/or require optional information to be copied into the WR
> structure.  Reject all those not explicitly handled so that we
> can't pass invalid information to drivers.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>  drivers/infiniband/core/uverbs_cmd.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Oh yes, this is absolutely needed for -stable.

Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

AFAIK, this path is rarely (never?) actually used. I think all the
drivers we have can post directly from userspace.

Jason

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote:
> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> 
> AFAIK, this path is rarely (never?) actually used. I think all the
> drivers we have can post directly from userspace.

Oh, interesting.  Is there any chance to deprecate it?  Not having
to care for the uvers command would really help with some of the
upcoming changes I have in my mind.

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Jason Gunthorpe]

On Wed, Aug 19, 2015 at 07:48:02PM +0200, Christoph Hellwig wrote:
> On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote:
> > Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > 
> > AFAIK, this path is rarely (never?) actually used. I think all the
> > drivers we have can post directly from userspace.
> 
> Oh, interesting.  Is there any chance to deprecate it?  Not having
> to care for the uvers command would really help with some of the
> upcoming changes I have in my mind.

Hmm, we'd need a survey of the userspace side to see if it is rarely
or never...

And we'd have to talk to the soft XXX guys to see if they plan to use
it..

I always like to see cruft go away, especially if it is broken cruft..

Jason

RE: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Hefty, Sean]

> AFAIK, this path is rarely (never?) actually used. I think all the
> drivers we have can post directly from userspace.

I didn't think the ipath or qib drivers post from userspace.

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Sagi Grimberg]

On 8/19/2015 8:54 PM, Jason Gunthorpe wrote:
> On Wed, Aug 19, 2015 at 07:48:02PM +0200, Christoph Hellwig wrote:
>> On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote:
>>> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>>
>>> AFAIK, this path is rarely (never?) actually used. I think all the
>>> drivers we have can post directly from userspace.
>>
>> Oh, interesting.  Is there any chance to deprecate it?  Not having
>> to care for the uvers command would really help with some of the
>> upcoming changes I have in my mind.
>
> Hmm, we'd need a survey of the userspace side to see if it is rarely
> or never...
>
> And we'd have to talk to the soft XXX guys to see if they plan to use
> it..

Checked in librxe (user-space softroce). Looks like posts are going via
this path...

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Sagi Grimberg]

On 8/19/2015 7:37 PM, Christoph Hellwig wrote:
> We have many WR opcodes that are only supported in kernel space
> and/or require optional information to be copied into the WR
> structure.  Reject all those not explicitly handled so that we
> can't pass invalid information to drivers.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/infiniband/core/uverbs_cmd.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index a15318a..f9f3921 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -2372,6 +2372,12 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
>   		next->send_flags = user_wr->send_flags;
>
>   		if (is_ud) {
> +			if (next->opcode != IB_WR_SEND &&
> +			    next->opcode != IB_WR_SEND_WITH_IMM) {
> +				ret = -EINVAL;
> +				goto out_put;
> +			}
> +
>   			next->wr.ud.ah = idr_read_ah(user_wr->wr.ud.ah,
>   						     file->ucontext);
>   			if (!next->wr.ud.ah) {
> @@ -2413,7 +2419,8 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
>   				next->wr.atomic.rkey = user_wr->wr.atomic.rkey;
>   				break;
>   			default:
> -				break;
> +				ret = -EINVAL;
> +				goto out_put;
>   			}
>   		}
>
>

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Haggai, can you also have a look?

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

On Wed, Aug 19, 2015 at 07:50:23PM +0000, Hefty, Sean wrote:
> > AFAIK, this path is rarely (never?) actually used. I think all the
> > drivers we have can post directly from userspace.
> 
> I didn't think the ipath or qib drivers post from userspace.

Makes sense with their software IB stack.  Guess the idea to get rid
of this path is dead, would have been too nice..

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Steve Wise]

On 8/20/2015 3:49 AM, Sagi Grimberg wrote:
> On 8/19/2015 8:54 PM, Jason Gunthorpe wrote:
>> On Wed, Aug 19, 2015 at 07:48:02PM +0200, Christoph Hellwig wrote:
>>> On Wed, Aug 19, 2015 at 11:46:14AM -0600, Jason Gunthorpe wrote:
>>>> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>>>
>>>> AFAIK, this path is rarely (never?) actually used. I think all the
>>>> drivers we have can post directly from userspace.
>>>
>>> Oh, interesting.  Is there any chance to deprecate it?  Not having
>>> to care for the uvers command would really help with some of the
>>> upcoming changes I have in my mind.
>>
>> Hmm, we'd need a survey of the userspace side to see if it is rarely
>> or never...
>>
>> And we'd have to talk to the soft XXX guys to see if they plan to use
>> it..
>
> Checked in librxe (user-space softroce). Looks like posts are going via
> this path...

Ditto for the soft iWARP stack, which is still out-of-linux.

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Haggai Eran]

On Thursday, August 20, 2015 11:52 AM, linux-rdma-owner@vger.kernel.org <linux-rdma-owner@vger.kernel.org> on behalf of Sagi Grimberg <sagig@dev.mellanox.co.il> wrote:
> On 8/19/2015 7:37 PM, Christoph Hellwig wrote:
>> We have many WR opcodes that are only supported in kernel space
>> and/or require optional information to be copied into the WR
>> structure.  Reject all those not explicitly handled so that we
>> can't pass invalid information to drivers.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   drivers/infiniband/core/uverbs_cmd.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
>> index a15318a..f9f3921 100644
>> --- a/drivers/infiniband/core/uverbs_cmd.c
>> +++ b/drivers/infiniband/core/uverbs_cmd.c
>> @@ -2372,6 +2372,12 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
>>               next->send_flags = user_wr->send_flags;
>>
>>               if (is_ud) {
>> +                     if (next->opcode != IB_WR_SEND &&
>> +                         next->opcode != IB_WR_SEND_WITH_IMM) {
>> +                             ret = -EINVAL;
>> +                             goto out_put;
>> +                     }
>> +
>>                       next->wr.ud.ah = idr_read_ah(user_wr->wr.ud.ah,
>>                                                    file->ucontext);
>>                       if (!next->wr.ud.ah) {
>> @@ -2413,7 +2419,8 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
>>                               next->wr.atomic.rkey = user_wr->wr.atomic.rkey;
>>                               break;
>>                       default:
>> -                             break;
>> +                             ret = -EINVAL;
>> +                             goto out_put;
>>                       }
>>               }
>>
>>
> 
> Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
> 
> Haggai, can you also have a look?

It looks like the default case in the non-UD branch is currently used to handle plain IB_WR_SEND operations, so the patch would cause these to return an error.

Haggai

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

On Sat, Aug 22, 2015 at 06:38:47AM +0000, Haggai Eran wrote:
> It looks like the default case in the non-UD branch is currently used to handle plain IB_WR_SEND operations, so the patch would cause these to return an error.

Indeed.  It's handled fine in patch 2 which splits up the case, but
will be incorrectly rejected with just this patch applied.

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Haggai Eran]

On 22/08/2015 11:25, Christoph Hellwig wrote:
> On Sat, Aug 22, 2015 at 06:38:47AM +0000, Haggai Eran wrote:
>> It looks like the default case in the non-UD branch is currently used to handle plain IB_WR_SEND operations, so the patch would cause these to return an error.
> 
> Indeed.  It's handled fine in patch 2 which splits up the case, but
> will be incorrectly rejected with just this patch applied.

Okay. Maybe you can just add a case for IB_WR_SEND in this patch to
avoid hurting bisectability.

Looking at the uverbs part in patch 2, I think the changes are okay. I
noticed there's a (__be32 __force) cast of the immediate data from
userspace (it was already in the existing code). I wonder, why not
define the field in the uapi struct as __be32 in the first place?

Haggai

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

On Mon, Aug 24, 2015 at 09:52:14AM +0300, Haggai Eran wrote:
> Okay. Maybe you can just add a case for IB_WR_SEND in this patch to
> avoid hurting bisectability.

I've done this already, just waiting for more feedback before resending:

http://git.infradead.org/users/hch/rdma.git/commitdiff/20f34ca8ecac302984f3a92b9ad29f5f4b41780d

> Looking at the uverbs part in patch 2, I think the changes are okay. I
> noticed there's a (__be32 __force) cast of the immediate data from
> userspace (it was already in the existing code). I wonder, why not
> define the field in the uapi struct as __be32 in the first place?

It looks odd to me as well, but it's not really something I want to
change in this series.  Note that sparse annoted types like __be32
aren't really common in userspace, but with a bit of effort they can
be supported.  We have them and regularly run sparse for xfsprogs for
example.

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Haggai Eran]

On 24/08/2015 09:55, Christoph Hellwig wrote:
> On Mon, Aug 24, 2015 at 09:52:14AM +0300, Haggai Eran wrote:
>> Okay. Maybe you can just add a case for IB_WR_SEND in this patch to
>> avoid hurting bisectability.
> 
> I've done this already, just waiting for more feedback before resending:
> 
> http://git.infradead.org/users/hch/rdma.git/commitdiff/20f34ca8ecac302984f3a92b9ad29f5f4b41780d

Great.

>> Looking at the uverbs part in patch 2, I think the changes are okay. I
>> noticed there's a (__be32 __force) cast of the immediate data from
>> userspace (it was already in the existing code). I wonder, why not
>> define the field in the uapi struct as __be32 in the first place?
> 
> It looks odd to me as well, but it's not really something I want to
> change in this series.  Note that sparse annoted types like __be32
> aren't really common in userspace, but with a bit of effort they can
> be supported.  We have them and regularly run sparse for xfsprogs for
> example.

I have to try it with libibverbs sometime. It doesn't use uapi yet
though IIRC - it has its own version of the header files.

Re: [PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

On Mon, Aug 24, 2015 at 10:59:21AM +0300, Haggai Eran wrote:
> > It looks odd to me as well, but it's not really something I want to
> > change in this series.  Note that sparse annoted types like __be32
> > aren't really common in userspace, but with a bit of effort they can
> > be supported.  We have them and regularly run sparse for xfsprogs for
> > example.
> 
> I have to try it with libibverbs sometime. It doesn't use uapi yet
> though IIRC - it has its own version of the header files.

Yes, I noticed that.  And the WR opcodes aren't even exported in the
uapi header, and use a shared namespace with the in-kernel only ones.

It's all a giant mess unfortunately.

[PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

We have many WR opcodes that are only supported in kernel space
and/or require optional information to be copied into the WR
structure.  Reject all those not explicitly handled so that we
can't pass invalid information to drivers.

Cc: stable@vger.kernel.org
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index a15318a..be4cb9f 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2372,6 +2372,12 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 		next->send_flags = user_wr->send_flags;
 
 		if (is_ud) {
+			if (next->opcode != IB_WR_SEND &&
+			    next->opcode != IB_WR_SEND_WITH_IMM) {
+				ret = -EINVAL;
+				goto out_put;
+			}
+
 			next->wr.ud.ah = idr_read_ah(user_wr->wr.ud.ah,
 						     file->ucontext);
 			if (!next->wr.ud.ah) {
@@ -2411,9 +2417,11 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 					user_wr->wr.atomic.compare_add;
 				next->wr.atomic.swap = user_wr->wr.atomic.swap;
 				next->wr.atomic.rkey = user_wr->wr.atomic.rkey;
+			case IB_WR_SEND:
 				break;
 			default:
-				break;
+				ret = -EINVAL;
+				goto out_put;
 			}
 		}
 
-- 
1.9.1