[PATCH] RDMA/core: Fix for parsing netlink string attribute

[PATCH] RDMA/core: Fix for parsing netlink string attribute

[Tatyana Nikolova]

The string iwpm_ulib_name is recorded in a nlmsg as a netlink attribute.
Without this fix parsing of the nlmsg by the userspace port mapper service fails
because of unknown attribute length, causing the port mapper service not to
register the client, which has sent the nlmsg.

Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com>
Cc: <stable@vger.kernel.org> #v3.16
---
 drivers/infiniband/core/iwpm_msg.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/core/iwpm_msg.c b/drivers/infiniband/core/iwpm_msg.c
index b85ddbc..e5558b2 100644
--- a/drivers/infiniband/core/iwpm_msg.c
+++ b/drivers/infiniband/core/iwpm_msg.c
@@ -33,7 +33,7 @@
 
 #include "iwpm_util.h"
 
-static const char iwpm_ulib_name[] = "iWarpPortMapperUser";
+static const char iwpm_ulib_name[IWPM_ULIBNAME_SIZE] = "iWarpPortMapperUser";
 static int iwpm_ulib_version = 3;
 static int iwpm_user_pid = IWPM_PID_UNDEFINED;
 static atomic_t echo_nlmsg_seq;
-- 
1.7.1

Re: [PATCH] RDMA/core: Fix for parsing netlink string attribute

[Jason Gunthorpe]

On Fri, May 08, 2015 at 04:36:33PM -0500, Tatyana Nikolova wrote:
> The string iwpm_ulib_name is recorded in a nlmsg as a netlink attribute.
> Without this fix parsing of the nlmsg by the userspace port mapper service fails
> because of unknown attribute length, causing the port mapper service not to
> register the client, which has sent the nlmsg.

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

This actually will copy some kernel memory to userspace. I think the
overflow is in .text, so probably not a security issue..

Jason

Re: [PATCH] RDMA/core: Fix for parsing netlink string attribute

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

On Fri, 2015-05-08 at 15:53 -0600, Jason Gunthorpe wrote:
> On Fri, May 08, 2015 at 04:36:33PM -0500, Tatyana Nikolova wrote:
> > The string iwpm_ulib_name is recorded in a nlmsg as a netlink attribute.
> > Without this fix parsing of the nlmsg by the userspace port mapper service fails
> > because of unknown attribute length, causing the port mapper service not to
> > register the client, which has sent the nlmsg.
> 
> Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> 
> This actually will copy some kernel memory to userspace. I think the
> overflow is in .text, so probably not a security issue..

It shouldn't be in the .text section.  This is defined as static const
char array, so it should be in one of the data sections.  And since we
are using an initializer smaller than the specific size of the array, I
would expect all of the unitialized bits to be 0.  But, the proof is in
the pudding.  A quick compile and check show us:

Contents of section .rodata:
 ...
 01e0 69576172 70506f72 744d6170 70657255  iWarpPortMapperU
 01f0 73657200 00000000 00000000 00000000  ser.............

Yep, just right.  It created a 32byte ro element, initialized it to 0,
then put our text in it.  This is exactly what will get copied in the nl
message and is precisely what we want.

Applied, thanks.

-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] RDMA/core: Fix for parsing netlink string attribute

[Jason Gunthorpe]

On Tue, May 12, 2015 at 01:14:26PM -0400, Doug Ledford wrote:
> On Fri, 2015-05-08 at 15:53 -0600, Jason Gunthorpe wrote:
> > On Fri, May 08, 2015 at 04:36:33PM -0500, Tatyana Nikolova wrote:
> > > The string iwpm_ulib_name is recorded in a nlmsg as a netlink attribute.
> > > Without this fix parsing of the nlmsg by the userspace port mapper service fails
> > > because of unknown attribute length, causing the port mapper service not to
> > > register the client, which has sent the nlmsg.
> > 
> > Reviewed-By: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > 
> > This actually will copy some kernel memory to userspace. I think the
> > overflow is in .text, so probably not a security issue..
> 
> It shouldn't be in the .text section.  

Pedantically, that is right, it is an archaic colloquialism to refer
to the entire set of post-link read-only sections as .text. (typically
the linker used to merge everything into .text)

I realize now I didn't consider modules when looking into this. No
time right now, can you check if there is any chance the read can
overflow past the page allocated to the module's .rodata?

> char array, so it should be in one of the data sections.  And since we
> are using an initializer smaller than the specific size of the array, I
> would expect all of the unitialized bits to be 0.  

I was talking about the situation before the patch. 

C defines a zero fill for incomplete initialization.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/core: Fix for parsing netlink string attribute

[Jason Gunthorpe]

On Tue, May 12, 2015 at 01:14:26PM -0400, Doug Ledford wrote:
> On Fri, 2015-05-08 at 15:53 -0600, Jason Gunthorpe wrote:
> > On Fri, May 08, 2015 at 04:36:33PM -0500, Tatyana Nikolova wrote:
> > > The string iwpm_ulib_name is recorded in a nlmsg as a netlink attribute.
> > > Without this fix parsing of the nlmsg by the userspace port mapper service fails
> > > because of unknown attribute length, causing the port mapper service not to
> > > register the client, which has sent the nlmsg.
> > 
> > Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > 
> > This actually will copy some kernel memory to userspace. I think the
> > overflow is in .text, so probably not a security issue..
> 
> It shouldn't be in the .text section.  

Pedantically, that is right, it is an archaic colloquialism to refer
to the entire set of post-link read-only sections as .text. (typically
the linker used to merge everything into .text)

I realize now I didn't consider modules when looking into this. No
time right now, can you check if there is any chance the read can
overflow past the page allocated to the module's .rodata?

> char array, so it should be in one of the data sections.  And since we
> are using an initializer smaller than the specific size of the array, I
> would expect all of the unitialized bits to be 0.  

I was talking about the situation before the patch. 

C defines a zero fill for incomplete initialization.

Jason

Re: [PATCH] RDMA/core: Fix for parsing netlink string attribute

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]

On Tue, 2015-05-12 at 11:50 -0600, Jason Gunthorpe wrote:
> On Tue, May 12, 2015 at 01:14:26PM -0400, Doug Ledford wrote:
> > On Fri, 2015-05-08 at 15:53 -0600, Jason Gunthorpe wrote:
> > > On Fri, May 08, 2015 at 04:36:33PM -0500, Tatyana Nikolova wrote:
> > > > The string iwpm_ulib_name is recorded in a nlmsg as a netlink attribute.
> > > > Without this fix parsing of the nlmsg by the userspace port mapper service fails
> > > > because of unknown attribute length, causing the port mapper service not to
> > > > register the client, which has sent the nlmsg.
> > > 
> > > Reviewed-By: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > 
> > > This actually will copy some kernel memory to userspace. I think the
> > > overflow is in .text, so probably not a security issue..
> > 
> > It shouldn't be in the .text section.  
> 
> Pedantically, that is right, it is an archaic colloquialism to refer
> to the entire set of post-link read-only sections as .text. (typically
> the linker used to merge everything into .text)
> 
> I realize now I didn't consider modules when looking into this. No
> time right now, can you check if there is any chance the read can
> overflow past the page allocated to the module's .rodata?
> 
> > char array, so it should be in one of the data sections.  And since we
> > are using an initializer smaller than the specific size of the array, I
> > would expect all of the unitialized bits to be 0.  
> 
> I was talking about the situation before the patch. 

Sorry, my misunderstanding.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] RDMA/core: Fix for parsing netlink string attribute

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1640 bytes --]

On Tue, 2015-05-12 at 11:50 -0600, Jason Gunthorpe wrote:
> On Tue, May 12, 2015 at 01:14:26PM -0400, Doug Ledford wrote:
> > On Fri, 2015-05-08 at 15:53 -0600, Jason Gunthorpe wrote:
> > > On Fri, May 08, 2015 at 04:36:33PM -0500, Tatyana Nikolova wrote:
> > > > The string iwpm_ulib_name is recorded in a nlmsg as a netlink attribute.
> > > > Without this fix parsing of the nlmsg by the userspace port mapper service fails
> > > > because of unknown attribute length, causing the port mapper service not to
> > > > register the client, which has sent the nlmsg.
> > > 
> > > Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > 
> > > This actually will copy some kernel memory to userspace. I think the
> > > overflow is in .text, so probably not a security issue..
> > 
> > It shouldn't be in the .text section.  
> 
> Pedantically, that is right, it is an archaic colloquialism to refer
> to the entire set of post-link read-only sections as .text. (typically
> the linker used to merge everything into .text)
> 
> I realize now I didn't consider modules when looking into this. No
> time right now, can you check if there is any chance the read can
> overflow past the page allocated to the module's .rodata?
> 
> > char array, so it should be in one of the data sections.  And since we
> > are using an initializer smaller than the specific size of the array, I
> > would expect all of the unitialized bits to be 0.  
> 
> I was talking about the situation before the patch. 

Sorry, my misunderstanding.

-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]