Re: [Cluster-devel] [PATCH v2] fs/dlm: Fix kernel memory disclosure

[Vlad Tsyrklevich]

Hello, I wanted to ping the list and see if this could get a review:

> Clear the 'unused' field and the uninitialized padding in 'lksb' to
> avoid leaking memory to userland in copy_result_to_user().
>
> Signed-off-by: Vlad Tsyrklevich <v...@tsyrklevich.net>
> ---
>  fs/dlm/user.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/dlm/user.c b/fs/dlm/user.c
> index 1ce908c..83ddd47 100644
> --- a/fs/dlm/user.c
> +++ b/fs/dlm/user.c
> @@ -122,6 +122,8 @@ static void compat_input(struct dlm_write_request *kb,
>  static void compat_output(struct dlm_lock_result *res,
>   struct dlm_lock_result32 *res32)
>  {
> +   memset(res32, 0, sizeof(*res32));
> +
> res32->version[0] = res->version[0];
> res32->version[1] = res->version[1];
> res32->version[2] = res->version[2];

Re: [Cluster-devel] [PATCH] fs/dlm: Fix kernel memory disclosure

[Vlad Tsyrklevich]

You're right, thanks for double checking that logic! I've just sent an
updated patch to the list.

On Thu, Jan 26, 2017 at 5:54 PM Steven Whitehouse <swhit...@redhat.com> wrote:
>
> Hi,
>
>
> On 26/01/17 08:54, Vlad Tsyrklevich wrote:
> > Hello, I wanted to ping the list and see if this could get a review.
> >
> > On Mon, Jan 9, 2017 at 8:27 PM, Vlad Tsyrklevich <v...@tsyrklevich.net> 
> > wrote:
> >> Clear the 'unused' field to avoid leaking memory to userland in
> >> copy_result_to_user().
> >>
> >> Signed-off-by: Vlad Tsyrklevich <v...@tsyrklevich.net>
> >> ---
> >>   fs/dlm/user.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/fs/dlm/user.c b/fs/dlm/user.c
> >> index 1ce908c..0570711 100644
> >> --- a/fs/dlm/user.c
> >> +++ b/fs/dlm/user.c
> >> @@ -138,6 +138,8 @@ static void compat_output(struct dlm_lock_result *res,
> >>  res32->lksb.sb_flags = res->lksb.sb_flags;
> >>  res32->lksb.sb_lkid = res->lksb.sb_lkid;
> >>  res32->lksb.sb_lvbptr = (__u32)(long)res->lksb.sb_lvbptr;
> >> +
> >> +   memset(>unused, 0, sizeof(res32->unused));
> >>   }
> >>   #endif
> >>
> >> --
> >> 2.7.0
> >>
> It looks like struct dlm_lksb32 has a hole in it, so it would be safer
> just to zero the whole of the dlm_lock_result32 before it is written to,
> rather than trying to find all the holes individually, even if slightly
> slower (I'm not sure it would be noticeable in reality though)
>
> Steve.
>

[Cluster-devel] [PATCH v2] fs/dlm: Fix kernel memory disclosure

[Vlad Tsyrklevich]

Clear the 'unused' field and the uninitialized padding in 'lksb' to
avoid leaking memory to userland in copy_result_to_user().

Signed-off-by: Vlad Tsyrklevich <v...@tsyrklevich.net>
---
 fs/dlm/user.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/dlm/user.c b/fs/dlm/user.c
index 1ce908c..83ddd47 100644
--- a/fs/dlm/user.c
+++ b/fs/dlm/user.c
@@ -122,6 +122,8 @@ static void compat_input(struct dlm_write_request *kb,
 static void compat_output(struct dlm_lock_result *res,
  struct dlm_lock_result32 *res32)
 {
+   memset(res32, 0, sizeof(*res32));
+
res32->version[0] = res->version[0];
res32->version[1] = res->version[1];
res32->version[2] = res->version[2];
-- 
2.7.0

Re: [Cluster-devel] [PATCH] fs/dlm: Fix kernel memory disclosure

[Vlad Tsyrklevich]

Hello, I wanted to ping the list and see if this could get a review.

On Mon, Jan 9, 2017 at 8:27 PM, Vlad Tsyrklevich <v...@tsyrklevich.net> wrote:
> Clear the 'unused' field to avoid leaking memory to userland in
> copy_result_to_user().
>
> Signed-off-by: Vlad Tsyrklevich <v...@tsyrklevich.net>
> ---
>  fs/dlm/user.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/dlm/user.c b/fs/dlm/user.c
> index 1ce908c..0570711 100644
> --- a/fs/dlm/user.c
> +++ b/fs/dlm/user.c
> @@ -138,6 +138,8 @@ static void compat_output(struct dlm_lock_result *res,
> res32->lksb.sb_flags = res->lksb.sb_flags;
> res32->lksb.sb_lkid = res->lksb.sb_lkid;
> res32->lksb.sb_lvbptr = (__u32)(long)res->lksb.sb_lvbptr;
> +
> +   memset(>unused, 0, sizeof(res32->unused));
>  }
>  #endif
>
> --
> 2.7.0
>

[Cluster-devel] [PATCH] fs/dlm: Fix kernel memory disclosure

[Vlad Tsyrklevich]

Clear the 'unused' field to avoid leaking memory to userland in
copy_result_to_user().

Signed-off-by: Vlad Tsyrklevich <v...@tsyrklevich.net>
---
 fs/dlm/user.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/dlm/user.c b/fs/dlm/user.c
index 1ce908c..0570711 100644
--- a/fs/dlm/user.c
+++ b/fs/dlm/user.c
@@ -138,6 +138,8 @@ static void compat_output(struct dlm_lock_result *res,
res32->lksb.sb_flags = res->lksb.sb_flags;
res32->lksb.sb_lkid = res->lksb.sb_lkid;
res32->lksb.sb_lvbptr = (__u32)(long)res->lksb.sb_lvbptr;
+
+   memset(>unused, 0, sizeof(res32->unused));
 }
 #endif
 
-- 
2.7.0