| Message ID | 20200728053604.404631-1-yepeilin.cs@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [Linux-kernel-mentees,net,v2] xdp: Prevent kernel-infoleak in xsk_getsockopt() | expand |
On Tue, 28 Jul 2020 at 07:37, Peilin Ye <yepeilin.cs@gmail.com> 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 <dan.carpenter@oracle.com> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> > --- Acked-by: Björn Töpel <bjorn.topel@intel.com> > 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: stable@vger.kernel.org" tag. (Suggested by Song Liu > <songliubraving@fb.com>) > - Initialize `stats` by assignment instead of using memset(). > (Suggested by Song Liu <songliubraving@fb.com>) > > 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 >
> On Jul 27, 2020, at 11:13 PM, Björn Töpel <bjorn.topel@gmail.com> wrote: > > On Tue, 28 Jul 2020 at 07:37, Peilin Ye <yepeilin.cs@gmail.com> 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 <dan.carpenter@oracle.com> >> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> >> --- > > Acked-by: Björn Töpel <bjorn.topel@intel.com> Acked-by: Song Liu <songliubraving@fb.com> [...]
On Tue, Jul 28, 2020 at 7:37 AM Peilin Ye <yepeilin.cs@gmail.com> 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 <dan.carpenter@oracle.com> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> Acked-by: Arnd Bergmann <arnd@arndb.de>
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 <dan.carpenter@oracle.com> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> > --- > 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!
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 <dan.carpenter@oracle.com> > > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> > > --- > > 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
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;
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 <dan.carpenter@oracle.com> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> --- 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: stable@vger.kernel.org" tag. (Suggested by Song Liu <songliubraving@fb.com>) - Initialize `stats` by assignment instead of using memset(). (Suggested by Song Liu <songliubraving@fb.com>) net/xdp/xsk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)