| Message ID | 20150508213633.GA13012@TENIKOLO-MOBL2 (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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;
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(-)