* [Linux-kernel-mentees] [PATCH net] xdp: Prevent kernel-infoleak in xsk_getsockopt()
@ 2020-07-28 2:28 Peilin Ye
2020-07-28 5:07 ` Song Liu
2020-07-28 5:36 ` [Linux-kernel-mentees] [PATCH net v2] " Peilin Ye
0 siblings, 2 replies; 9+ messages in thread
From: Peilin Ye @ 2020-07-28 2:28 UTC (permalink / raw)
To: Björn Töpel, Magnus Karlsson, Jonathan Lemon
Cc: Song Liu, Martin KaFai Lau, linux-kernel, Daniel Borkmann,
Arnd Bergmann, John Fastabend, Alexei Starovoitov,
David S. Miller, linux-kernel-mentees, netdev, Yonghong Song,
KP Singh, Jakub Kicinski, bpf, Andrii Nakryiko, Peilin Ye,
Dan Carpenter, Jesper Dangaard Brouer
xsk_getsockopt() is copying uninitialized stack memory to userspace when
`extra_stats` is `false`. Fix it by initializing `stats` with memset().
Cc: stable@vger.kernel.org
Fixes: 8aa5a33578e9 ("xsk: Add new statistics")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
---
net/xdp/xsk.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 26e3bba8c204..acf001908a0d 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -844,6 +844,8 @@ static int xsk_getsockopt(struct socket *sock, int level, int optname,
bool extra_stats = true;
size_t stats_size;
+ memset(&stats, 0, sizeof(stats));
+
if (len < sizeof(struct xdp_statistics_v1)) {
return -EINVAL;
} else if (len < sizeof(stats)) {
--
2.25.1
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH net] xdp: Prevent kernel-infoleak in xsk_getsockopt()
2020-07-28 2:28 [Linux-kernel-mentees] [PATCH net] xdp: Prevent kernel-infoleak in xsk_getsockopt() Peilin Ye
@ 2020-07-28 5:07 ` Song Liu
2020-07-28 5:25 ` Peilin Ye
2020-07-28 5:36 ` [Linux-kernel-mentees] [PATCH net v2] " Peilin Ye
1 sibling, 1 reply; 9+ messages in thread
From: Song Liu @ 2020-07-28 5:07 UTC (permalink / raw)
To: Peilin Ye
Cc: Song Liu, open list, Daniel Borkmann, Arnd Bergmann,
John Fastabend, Alexei Starovoitov, Martin KaFai Lau,
Yonghong Song, linux-kernel-mentees, Networking, Magnus Karlsson,
Jonathan Lemon, KP Singh, Jakub Kicinski, bpf,
Björn Töpel, Andrii Nakryiko, David S. Miller,
Dan Carpenter, Jesper Dangaard Brouer
On Mon, Jul 27, 2020 at 7:30 PM Peilin Ye <yepeilin.cs@gmail.com> wrote:
>
> xsk_getsockopt() is copying uninitialized stack memory to userspace when
> `extra_stats` is `false`. Fix it by initializing `stats` with memset().
>
> Cc: stable@vger.kernel.org
8aa5a33578e9 is not in stable branches yet, so we don't need to Cc stable.
> Fixes: 8aa5a33578e9 ("xsk: Add new statistics")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
> net/xdp/xsk.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 26e3bba8c204..acf001908a0d 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -844,6 +844,8 @@ static int xsk_getsockopt(struct socket *sock, int level, int optname,
> bool extra_stats = true;
> size_t stats_size;
>
> + memset(&stats, 0, sizeof(stats));
> +
xsk.c doesn't include linux/string.h directly, so using memset may break
build for some config combinations. We can probably just use
struct xdp_statistics stats = {};
Thanks,
Song
> if (len < sizeof(struct xdp_statistics_v1)) {
> return -EINVAL;
> } else if (len < sizeof(stats)) {
> --
> 2.25.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH net] xdp: Prevent kernel-infoleak in xsk_getsockopt()
2020-07-28 5:07 ` Song Liu
@ 2020-07-28 5:25 ` Peilin Ye
0 siblings, 0 replies; 9+ messages in thread
From: Peilin Ye @ 2020-07-28 5:25 UTC (permalink / raw)
To: Song Liu
Cc: Song Liu, open list, Daniel Borkmann, Arnd Bergmann,
John Fastabend, Alexei Starovoitov, Martin KaFai Lau,
Yonghong Song, linux-kernel-mentees, Networking, Magnus Karlsson,
Jonathan Lemon, KP Singh, Jakub Kicinski, bpf,
Björn Töpel, Andrii Nakryiko, David S. Miller,
Dan Carpenter, Jesper Dangaard Brouer
On Mon, Jul 27, 2020 at 10:07:20PM -0700, Song Liu wrote:
> On Mon, Jul 27, 2020 at 7:30 PM Peilin Ye <yepeilin.cs@gmail.com> wrote:
> >
> > xsk_getsockopt() is copying uninitialized stack memory to userspace when
> > `extra_stats` is `false`. Fix it by initializing `stats` with memset().
> >
> > Cc: stable@vger.kernel.org
>
> 8aa5a33578e9 is not in stable branches yet, so we don't need to Cc stable.
>
> > Fixes: 8aa5a33578e9 ("xsk: Add new statistics")
> > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> > ---
> > net/xdp/xsk.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 26e3bba8c204..acf001908a0d 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -844,6 +844,8 @@ static int xsk_getsockopt(struct socket *sock, int level, int optname,
> > bool extra_stats = true;
> > size_t stats_size;
> >
> > + memset(&stats, 0, sizeof(stats));
> > +
>
> xsk.c doesn't include linux/string.h directly, so using memset may break
> build for some config combinations. We can probably just use
>
> struct xdp_statistics stats = {};
I see. I will send v2 soon. Thank you for reviewing the patch!
Peilin Ye
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
2020-07-28 2:28 [Linux-kernel-mentees] [PATCH net] xdp: Prevent kernel-infoleak in xsk_getsockopt() Peilin Ye
2020-07-28 5:07 ` Song Liu
@ 2020-07-28 5:36 ` Peilin Ye
2020-07-28 6:13 ` Björn Töpel
` (2 more replies)
1 sibling, 3 replies; 9+ messages in thread
From: Peilin Ye @ 2020-07-28 5:36 UTC (permalink / raw)
To: Song Liu, Björn Töpel, Magnus Karlsson, Jonathan Lemon
Cc: Martin KaFai Lau, linux-kernel, Daniel Borkmann, Arnd Bergmann,
John Fastabend, Alexei Starovoitov, David S. Miller,
linux-kernel-mentees, netdev, Yonghong Song, KP Singh,
Jakub Kicinski, bpf, Andrii Nakryiko, Peilin Ye, Dan Carpenter,
Jesper Dangaard Brouer
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(-)
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 mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
2020-07-28 5:36 ` [Linux-kernel-mentees] [PATCH net v2] " Peilin Ye
@ 2020-07-28 6:13 ` Björn Töpel
2020-07-28 6:15 ` Song Liu via Linux-kernel-mentees
2020-07-28 7:34 ` Arnd Bergmann
2020-07-28 10:53 ` Daniel Borkmann
2 siblings, 1 reply; 9+ messages in thread
From: Björn Töpel @ 2020-07-28 6:13 UTC (permalink / raw)
To: Peilin Ye
Cc: Song Liu, LKML, Daniel Borkmann, Arnd Bergmann, John Fastabend,
Alexei Starovoitov, Martin KaFai Lau, Yonghong Song,
linux-kernel-mentees, Netdev, Magnus Karlsson, Jonathan Lemon,
KP Singh, Jakub Kicinski, bpf, Björn Töpel,
Andrii Nakryiko, David S. Miller, Dan Carpenter,
Jesper Dangaard Brouer
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
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
2020-07-28 6:13 ` Björn Töpel
@ 2020-07-28 6:15 ` Song Liu via Linux-kernel-mentees
0 siblings, 0 replies; 9+ messages in thread
From: Song Liu via Linux-kernel-mentees @ 2020-07-28 6:15 UTC (permalink / raw)
To: Björn Töpel
Cc: Martin Lau, LKML, Daniel Borkmann, Arnd Bergmann, John Fastabend,
Alexei Starovoitov, David S. Miller, Yonghong Song,
linux-kernel-mentees, Netdev, Magnus Karlsson, Jonathan Lemon,
KP Singh, Jakub Kicinski, bpf, Björn Töpel,
Andrii Nakryiko, Peilin Ye, Dan Carpenter,
Jesper Dangaard Brouer
> 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>
[...]
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
2020-07-28 5:36 ` [Linux-kernel-mentees] [PATCH net v2] " Peilin Ye
2020-07-28 6:13 ` Björn Töpel
@ 2020-07-28 7:34 ` Arnd Bergmann
2020-07-28 10:53 ` Daniel Borkmann
2 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2020-07-28 7:34 UTC (permalink / raw)
To: Peilin Ye
Cc: Song Liu, linux-kernel, Jesper Dangaard Brouer, Daniel Borkmann,
John Fastabend, Alexei Starovoitov, Martin KaFai Lau,
Yonghong Song, Networking, Magnus Karlsson, Jonathan Lemon,
KP Singh, Jakub Kicinski, bpf, Björn Töpel,
Andrii Nakryiko, David S. Miller, Dan Carpenter,
linux-kernel-mentees
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>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
2020-07-28 5:36 ` [Linux-kernel-mentees] [PATCH net v2] " Peilin Ye
2020-07-28 6:13 ` Björn Töpel
2020-07-28 7:34 ` Arnd Bergmann
@ 2020-07-28 10:53 ` Daniel Borkmann
2020-07-28 11:07 ` Peilin Ye
2 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2020-07-28 10:53 UTC (permalink / raw)
To: Peilin Ye, Song Liu, Björn Töpel, Magnus Karlsson,
Jonathan Lemon
Cc: linux-kernel, Jesper Dangaard Brouer, Arnd Bergmann,
John Fastabend, Alexei Starovoitov, Martin KaFai Lau, netdev,
Yonghong Song, KP Singh, Jakub Kicinski, bpf, Andrii Nakryiko,
David S. Miller, Dan Carpenter, linux-kernel-mentees
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!
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH net v2] xdp: Prevent kernel-infoleak in xsk_getsockopt()
2020-07-28 10:53 ` Daniel Borkmann
@ 2020-07-28 11:07 ` Peilin Ye
0 siblings, 0 replies; 9+ messages in thread
From: Peilin Ye @ 2020-07-28 11:07 UTC (permalink / raw)
To: Daniel Borkmann, Song Liu, Björn Töpel,
Magnus Karlsson, Jonathan Lemon
Cc: linux-kernel, Jesper Dangaard Brouer, Arnd Bergmann,
John Fastabend, Alexei Starovoitov, Martin KaFai Lau, netdev,
Yonghong Song, KP Singh, Jakub Kicinski, bpf, Andrii Nakryiko,
David S. Miller, Dan Carpenter, linux-kernel-mentees
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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-07-28 11:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 2:28 [Linux-kernel-mentees] [PATCH net] xdp: Prevent kernel-infoleak in xsk_getsockopt() Peilin Ye
2020-07-28 5:07 ` Song Liu
2020-07-28 5:25 ` Peilin Ye
2020-07-28 5:36 ` [Linux-kernel-mentees] [PATCH net v2] " Peilin Ye
2020-07-28 6:13 ` Björn Töpel
2020-07-28 6:15 ` Song Liu via Linux-kernel-mentees
2020-07-28 7:34 ` Arnd Bergmann
2020-07-28 10:53 ` Daniel Borkmann
2020-07-28 11:07 ` Peilin Ye
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).