* Re: [PATCH v4] add the option of fortified string.h functions [not found] <20170526095404.20439-1-danielmicay@gmail.com> @ 2017-06-02 21:07 ` Andrew Morton [not found] ` <20170602140743.274b9babba6118bfd12c7a26-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> 0 siblings, 1 reply; 3+ messages in thread From: Andrew Morton @ 2017-06-02 21:07 UTC (permalink / raw) To: Daniel Micay Cc: linux-mm, Kees Cook, kernel-hardening, linux-kernel, Mark Rutland, Daniel Axtens, Moni Shoua, Doug Ledford, Sean Hefty, Hal Rosenstock, linux-rdma On Fri, 26 May 2017 05:54:04 -0400 Daniel Micay <danielmicay@gmail.com> wrote: > This adds support for compiling with a rough equivalent to the glibc > _FORTIFY_SOURCE=1 feature, providing compile-time and runtime buffer > overflow checks for string.h functions when the compiler determines the > size of the source or destination buffer at compile-time. Unlike glibc, > it covers buffer reads in addition to writes. Did we find a bug in drivers/infiniband/sw/rxe/rxe_resp.c? i386 allmodconfig: In file included from ./include/linux/bitmap.h:8:0, from ./include/linux/cpumask.h:11, from ./include/linux/mm_types_task.h:13, from ./include/linux/mm_types.h:4, from ./include/linux/kmemcheck.h:4, from ./include/linux/skbuff.h:18, from drivers/infiniband/sw/rxe/rxe_resp.c:34: In function 'memcpy', inlined from 'send_atomic_ack.constprop' at drivers/infiniband/sw/rxe/rxe_resp.c:998:2, inlined from 'acknowledge' at drivers/infiniband/sw/rxe/rxe_resp.c:1026:3, inlined from 'rxe_responder' at drivers/infiniband/sw/rxe/rxe_resp.c:1286:10: ./include/linux/string.h:309:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter __read_overflow2(); If so, can you please interpret this for the infiniband developers? -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <20170602140743.274b9babba6118bfd12c7a26-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>]
* Re: [PATCH v4] add the option of fortified string.h functions [not found] ` <20170602140743.274b9babba6118bfd12c7a26-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> @ 2017-06-02 21:32 ` Daniel Micay [not found] ` <1496439121.13303.1.camel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 3+ messages in thread From: Daniel Micay @ 2017-06-02 21:32 UTC (permalink / raw) To: Andrew Morton Cc: linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Kees Cook, kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, linux-kernel, Mark Rutland, Daniel Axtens, Moni Shoua, Doug Ledford, Sean Hefty, Hal Rosenstock, linux-rdma-u79uwXL29TY76Z2rM5mHXA On Fri, 2017-06-02 at 14:07 -0700, Andrew Morton wrote: > On Fri, 26 May 2017 05:54:04 -0400 Daniel Micay <danielmicay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org > > wrote: > > > This adds support for compiling with a rough equivalent to the glibc > > _FORTIFY_SOURCE=1 feature, providing compile-time and runtime buffer > > overflow checks for string.h functions when the compiler determines > > the > > size of the source or destination buffer at compile-time. Unlike > > glibc, > > it covers buffer reads in addition to writes. > > Did we find a bug in drivers/infiniband/sw/rxe/rxe_resp.c? > > i386 allmodconfig: > > In file included from ./include/linux/bitmap.h:8:0, > from ./include/linux/cpumask.h:11, > from ./include/linux/mm_types_task.h:13, > from ./include/linux/mm_types.h:4, > from ./include/linux/kmemcheck.h:4, > from ./include/linux/skbuff.h:18, > from drivers/infiniband/sw/rxe/rxe_resp.c:34: > In function 'memcpy', > inlined from 'send_atomic_ack.constprop' at > drivers/infiniband/sw/rxe/rxe_resp.c:998:2, > inlined from 'acknowledge' at > drivers/infiniband/sw/rxe/rxe_resp.c:1026:3, > inlined from 'rxe_responder' at > drivers/infiniband/sw/rxe/rxe_resp.c:1286:10: > ./include/linux/string.h:309:4: error: call to '__read_overflow2' > declared with attribute error: detected read beyond size of object > passed as 2nd parameter > __read_overflow2(); > > > If so, can you please interpret this for the infiniband developers? It copies sizeof(skb->cb) bytes with memcpy which is 48 bytes since cb is a 48 byte char array in `struct sk_buff`. The source buffer is a `struct rxe_pkt_info`: struct rxe_pkt_info { struct rxe_dev *rxe; /* device that owns packet */ struct rxe_qp *qp; /* qp that owns packet */ struct rxe_send_wqe *wqe; /* send wqe */ u8 *hdr; /* points to bth */ u32 mask; /* useful info about pkt */ u32 psn; /* bth psn of packet */ u16 pkey_index; /* partition of pkt */ u16 paylen; /* length of bth - icrc */ u8 port_num; /* port pkt received on */ u8 opcode; /* bth opcode of packet */ u8 offset; /* bth offset from pkt->hdr */ }; That looks like 32 bytes (1 byte of padding) on 32-bit and 48 bytes on 64-bit (1 byte of padding), so on 32-bit there's a read overflow of 16 bytes from the stack here. -- 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <1496439121.13303.1.camel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH v4] add the option of fortified string.h functions [not found] ` <1496439121.13303.1.camel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2017-06-03 5:07 ` Kees Cook 0 siblings, 0 replies; 3+ messages in thread From: Kees Cook @ 2017-06-03 5:07 UTC (permalink / raw) To: Andrew Morton, Moni Shoua, Doug Ledford, Sean Hefty, Hal Rosenstock Cc: Daniel Micay, Linux-MM, kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8@public.gmane.org, linux-kernel, Mark Rutland, Daniel Axtens, linux-rdma-u79uwXL29TY76Z2rM5mHXA On Fri, Jun 2, 2017 at 2:32 PM, Daniel Micay <danielmicay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > On Fri, 2017-06-02 at 14:07 -0700, Andrew Morton wrote: >> On Fri, 26 May 2017 05:54:04 -0400 Daniel Micay <danielmicay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org >> > wrote: >> >> > This adds support for compiling with a rough equivalent to the glibc >> > _FORTIFY_SOURCE=1 feature, providing compile-time and runtime buffer >> > overflow checks for string.h functions when the compiler determines >> > the >> > size of the source or destination buffer at compile-time. Unlike >> > glibc, >> > it covers buffer reads in addition to writes. >> >> Did we find a bug in drivers/infiniband/sw/rxe/rxe_resp.c? >> >> i386 allmodconfig: >> >> In file included from ./include/linux/bitmap.h:8:0, >> from ./include/linux/cpumask.h:11, >> from ./include/linux/mm_types_task.h:13, >> from ./include/linux/mm_types.h:4, >> from ./include/linux/kmemcheck.h:4, >> from ./include/linux/skbuff.h:18, >> from drivers/infiniband/sw/rxe/rxe_resp.c:34: >> In function 'memcpy', >> inlined from 'send_atomic_ack.constprop' at >> drivers/infiniband/sw/rxe/rxe_resp.c:998:2, >> inlined from 'acknowledge' at >> drivers/infiniband/sw/rxe/rxe_resp.c:1026:3, >> inlined from 'rxe_responder' at >> drivers/infiniband/sw/rxe/rxe_resp.c:1286:10: >> ./include/linux/string.h:309:4: error: call to '__read_overflow2' >> declared with attribute error: detected read beyond size of object >> passed as 2nd parameter >> __read_overflow2(); >> >> >> If so, can you please interpret this for the infiniband developers? > > It copies sizeof(skb->cb) bytes with memcpy which is 48 bytes since cb > is a 48 byte char array in `struct sk_buff`. The source buffer is a > `struct rxe_pkt_info`: > > struct rxe_pkt_info { > struct rxe_dev *rxe; /* device that owns packet */ > struct rxe_qp *qp; /* qp that owns packet */ > struct rxe_send_wqe *wqe; /* send wqe */ > u8 *hdr; /* points to bth */ > u32 mask; /* useful info about pkt */ > u32 psn; /* bth psn of packet */ > u16 pkey_index; /* partition of pkt */ > u16 paylen; /* length of bth - icrc */ > u8 port_num; /* port pkt received on */ > u8 opcode; /* bth opcode of packet */ > u8 offset; /* bth offset from pkt->hdr */ > }; > > That looks like 32 bytes (1 byte of padding) on 32-bit and 48 bytes on > 64-bit (1 byte of padding), so on 32-bit there's a read overflow of 16 > bytes from the stack here. This should work (untested): diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c index 23039768f541..7b226deb83bb 100644 --- a/drivers/infiniband/sw/rxe/rxe_resp.c +++ b/drivers/infiniband/sw/rxe/rxe_resp.c @@ -995,7 +995,9 @@ static int send_atomic_ack(struct rxe_qp *qp, struct rxe_pkt_info *pkt, free_rd_atomic_resource(qp, res); rxe_advance_resp_resource(qp); - memcpy(SKB_TO_PKT(skb), &ack_pkt, sizeof(skb->cb)); + memcpy(SKB_TO_PKT(skb), &ack_pkt, sizeof(ack_ptr)); + memset(SKB_TO_PKT(skb) + sizeof(ack_ptr), 0, + sizeof(skb->cb) - sizeof(ack_ptr)); res->type = RXE_ATOMIC_MASK; res->atomic.skb = skb; Andrew, there are other fortify fixes too: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/fortify&id=af6b0151896240457ef0fdc18ace533c3d3fbb75 https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/fortify&id=186eaf81b43bf90d6b533732fb11ad31ca27df9d https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/fortify&id=95d589f21b3aef757f0eb3d0224b78648a4b22d2 https://github.com/thestinger/linux-hardened/commit/576e64469b0c4634c007445c5f16bfde610b3600 Do you want me to resend these for you to carry, or reping maintainers? Other fixes have already landed in -next. (And there are two arm64 fixes, too.) -Kees -- Kees Cook Pixel Security -- 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-06-03 5:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20170526095404.20439-1-danielmicay@gmail.com>
2017-06-02 21:07 ` [PATCH v4] add the option of fortified string.h functions Andrew Morton
[not found] ` <20170602140743.274b9babba6118bfd12c7a26-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
2017-06-02 21:32 ` Daniel Micay
[not found] ` <1496439121.13303.1.camel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-06-03 5:07 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox