Re: [Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
On Tue, Jul 28, 2020 at 12:53:59PM +0200, Daniel Borkmann wrote: > On 7/28/20 7:36 AM, Peilin Ye wrote: > > xsk_getsockopt() is copying uninitialized stack memory to userspace when > > `extra_stats` is `false`. Fix it. > > > > Fixes: 8aa5a33578e9 ("xsk: Add new statistics") > > Suggested-by: Dan Carpenter > > Signed-off-by: Peilin Ye > > --- > > Doing `= {};` is sufficient since currently `struct xdp_statistics` is > > defined as follows: > > > > struct xdp_statistics { > > __u64 rx_dropped; > > __u64 rx_invalid_descs; > > __u64 tx_invalid_descs; > > __u64 rx_ring_full; > > __u64 rx_fill_ring_empty_descs; > > __u64 tx_ring_empty_descs; > > }; > > > > When being copied to the userspace, `stats` will not contain any > > uninitialized "holes" between struct fields. > > I've added above explanation to the commit log since it's useful reasoning > for later > on 'why' something has been done a certain way. Applied, thanks Peilin! Ah, I see. Thank you for reviewing the patch! Peilin Ye
Re: [Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
On 7/28/20 7:36 AM, Peilin Ye wrote: xsk_getsockopt() is copying uninitialized stack memory to userspace when `extra_stats` is `false`. Fix it. Fixes: 8aa5a33578e9 ("xsk: Add new statistics") Suggested-by: Dan Carpenter Signed-off-by: Peilin Ye --- Doing `= {};` is sufficient since currently `struct xdp_statistics` is defined as follows: struct xdp_statistics { __u64 rx_dropped; __u64 rx_invalid_descs; __u64 tx_invalid_descs; __u64 rx_ring_full; __u64 rx_fill_ring_empty_descs; __u64 tx_ring_empty_descs; }; When being copied to the userspace, `stats` will not contain any uninitialized "holes" between struct fields. I've added above explanation to the commit log since it's useful reasoning for later on 'why' something has been done a certain way. Applied, thanks Peilin!
Re: [Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
On Tue, Jul 28, 2020 at 7:37 AM Peilin Ye wrote: > > xsk_getsockopt() is copying uninitialized stack memory to userspace when > `extra_stats` is `false`. Fix it. > > Fixes: 8aa5a33578e9 ("xsk: Add new statistics") > Suggested-by: Dan Carpenter > Signed-off-by: Peilin Ye Acked-by: Arnd Bergmann
Re: [Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
> On Jul 27, 2020, at 11:13 PM, Björn Töpel wrote: > > On Tue, 28 Jul 2020 at 07:37, Peilin Ye wrote: >> >> xsk_getsockopt() is copying uninitialized stack memory to userspace when >> `extra_stats` is `false`. Fix it. >> >> Fixes: 8aa5a33578e9 ("xsk: Add new statistics") >> Suggested-by: Dan Carpenter >> Signed-off-by: Peilin Ye >> --- > > Acked-by: Björn Töpel Acked-by: Song Liu [...]
Re: [Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
On Tue, 28 Jul 2020 at 07:37, Peilin Ye wrote: > > xsk_getsockopt() is copying uninitialized stack memory to userspace when > `extra_stats` is `false`. Fix it. > > Fixes: 8aa5a33578e9 ("xsk: Add new statistics") > Suggested-by: Dan Carpenter > Signed-off-by: Peilin Ye > --- Acked-by: Björn Töpel > Doing `= {};` is sufficient since currently `struct xdp_statistics` is > defined as follows: > > struct xdp_statistics { > __u64 rx_dropped; > __u64 rx_invalid_descs; > __u64 tx_invalid_descs; > __u64 rx_ring_full; > __u64 rx_fill_ring_empty_descs; > __u64 tx_ring_empty_descs; > }; > > When being copied to the userspace, `stats` will not contain any > uninitialized "holes" between struct fields. > > Changes in v2: > - Remove the "Cc: sta...@vger.kernel.org" tag. (Suggested by Song Liu > ) > - Initialize `stats` by assignment instead of using memset(). > (Suggested by Song Liu ) > > net/xdp/xsk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > index 26e3bba8c204..b2b533eddebf 100644 > --- a/net/xdp/xsk.c > +++ b/net/xdp/xsk.c > @@ -840,7 +840,7 @@ static int xsk_getsockopt(struct socket *sock, int level, > int optname, > switch (optname) { > case XDP_STATISTICS: > { > - struct xdp_statistics stats; > + struct xdp_statistics stats = {}; > bool extra_stats = true; > size_t stats_size; > > -- > 2.25.1 >
[Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
xsk_getsockopt() is copying uninitialized stack memory to userspace when `extra_stats` is `false`. Fix it. Fixes: 8aa5a33578e9 ("xsk: Add new statistics") Suggested-by: Dan Carpenter Signed-off-by: Peilin Ye --- Doing `= {};` is sufficient since currently `struct xdp_statistics` is defined as follows: struct xdp_statistics { __u64 rx_dropped; __u64 rx_invalid_descs; __u64 tx_invalid_descs; __u64 rx_ring_full; __u64 rx_fill_ring_empty_descs; __u64 tx_ring_empty_descs; }; When being copied to the userspace, `stats` will not contain any uninitialized "holes" between struct fields. Changes in v2: - Remove the "Cc: sta...@vger.kernel.org" tag. (Suggested by Song Liu ) - Initialize `stats` by assignment instead of using memset(). (Suggested by Song Liu ) net/xdp/xsk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c index 26e3bba8c204..b2b533eddebf 100644 --- a/net/xdp/xsk.c +++ b/net/xdp/xsk.c @@ -840,7 +840,7 @@ static int xsk_getsockopt(struct socket *sock, int level, int optname, switch (optname) { case XDP_STATISTICS: { - struct xdp_statistics stats; + struct xdp_statistics stats = {}; bool extra_stats = true; size_t stats_size; -- 2.25.1