[Disco][SRU][CVE-2019-2214] Fix for Binder OOB write

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[Disco][SRU][CVE-2019-2214] Fix for Binder OOB write

Connor Kuehl
https://people.canonical.com/~ubuntu-security/cve/2019/CVE-2019-2214.html

From the link above:

    "In binder_transaction of binder.c, there is a possible out of bounds write
    due to a missing bounds check. This could lead to local escalation of
    privilege with no additional execution privileges needed. User interaction
    is not needed for exploitation.Product: AndroidVersions: Android
    kernelAndroid ID: A-136210786References: Upstream kernel"

These backport notes are also included on the patch:

There is a much larger cleanup patch (that Disco does not have) that moves
away from using pointers and pointer arithmetic to using uintptr values,
that patch is bde4a19fc04f ("binder: use userspace pointer as base of
buffer space"). I used that patch to create a mapping of equivalent
variables:

  sg_bufp    => sg_buf_offset
  sg_buf_end => sg_buf_end_offset
  offp       => buffer_offset
  off_start  => off_start_offset
  off_end    => off_end_offset

to construct an equivalent patch for this CVE without pulling in the
other larger patch to base this on.

No new compiler warnings for the android driver.

@@ -3239,7 +3239,8 @@ static void binder_transaction(struct binder_proc *proc,      
    buffer_offset = off_start_offset;                                              
    off_end_offset = off_start_offset + tr->offsets_size;                          
    sg_buf_offset = ALIGN(off_end_offset, sizeof(void *));                          
-   sg_buf_end_offset = sg_buf_offset + extra_buffers_size;                        
+   sg_buf_end_offset = sg_buf_offset + extra_buffers_size -                        
+       ALIGN(secctx_sz, sizeof(u64));                                              
    off_min = 0;                                                                    
    for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;          
         buffer_offset += sizeof(binder_size_t)) {

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Disco][SRU][PATCH] binder: Set end of SG buffer area properly.

Connor Kuehl
From: Martijn Coenen <[hidden email]>

CVE-2019-2214

In case the target node requests a security context, the
extra_buffers_size is increased with the size of the security context.
But, that size is not available for use by regular scatter-gather
buffers; make sure the ending of that buffer is marked correctly.

Acked-by: Todd Kjos <[hidden email]>
Fixes: ec74136ded79 ("binder: create node flag to request sender's security context")
Signed-off-by: Martijn Coenen <[hidden email]>
Cc: [hidden email] # 5.1+
Link: https://lore.kernel.org/r/20190709110923.220736-1-maco@...
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(backported from commit a56587065094fd96eb4c2b5ad65571daad32156d)
[ Connor Kuehl: there is a much larger cleanup patch that converts away
  from using pointers and pointer arithmetic to using uintptr values,
  that patch is bde4a19fc04f ("binder: use userspace pointer as base of
  buffer space"). I used that patch to create a mapping of equivalent
  variables:

    sg_bufp    => sg_buf_offset
    sg_buf_end => sg_buf_end_offset
    offp       => buffer_offset
    off_start  => off_start_offset
    off_end    => off_end_offset

  to construct an equivalent patch for this CVE without pulling in the
  other larger patch to base this on.

@@ -3239,7 +3239,8 @@ static void binder_transaction(struct binder_proc *proc,      
    buffer_offset = off_start_offset;                                              
    off_end_offset = off_start_offset + tr->offsets_size;                          
    sg_buf_offset = ALIGN(off_end_offset, sizeof(void *));                          
-   sg_buf_end_offset = sg_buf_offset + extra_buffers_size;                        
+   sg_buf_end_offset = sg_buf_offset + extra_buffers_size -                        
+       ALIGN(secctx_sz, sizeof(u64));                                              
    off_min = 0;                                                                    
    for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;          
         buffer_offset += sizeof(binder_size_t)) { ]
Signed-off-by: Connor Kuehl <[hidden email]>
---
 drivers/android/binder.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 6369f4ffab12..507759449ea2 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3130,7 +3130,8 @@ static void binder_transaction(struct binder_proc *proc,
  }
  off_end = (void *)off_start + tr->offsets_size;
  sg_bufp = (u8 *)(PTR_ALIGN(off_end, sizeof(void *)));
- sg_buf_end = sg_bufp + extra_buffers_size;
+ sg_buf_end = sg_bufp + extra_buffers_size -
+ ALIGN(secctx_sz, sizeof(u64));
  off_min = 0;
  for (; offp < off_end; offp++) {
  struct binder_object_header *hdr;
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK/cmnt: [Disco][SRU][CVE-2019-2214] Fix for Binder OOB write

Kamal Mostafa-2
In reply to this post by Connor Kuehl
Backport LGTM, so...

Acked-by: Kamal Mostafa <[hidden email]>

IMHO, your backport note in the commit does not need to contain such a
detailed picture of the backport (nor the whole original commit!).
A reviewer is likely going to go examine the actual upstream commit
anyway.  :-)

 -Kamal

On Tue, Nov 26, 2019 at 02:42:30PM -0800, Connor Kuehl wrote:

> https://people.canonical.com/~ubuntu-security/cve/2019/CVE-2019-2214.html
>
> From the link above:
>
>     "In binder_transaction of binder.c, there is a possible out of bounds write
>     due to a missing bounds check. This could lead to local escalation of
>     privilege with no additional execution privileges needed. User interaction
>     is not needed for exploitation.Product: AndroidVersions: Android
>     kernelAndroid ID: A-136210786References: Upstream kernel"
>
> These backport notes are also included on the patch:
>
> There is a much larger cleanup patch (that Disco does not have) that moves
> away from using pointers and pointer arithmetic to using uintptr values,
> that patch is bde4a19fc04f ("binder: use userspace pointer as base of
> buffer space"). I used that patch to create a mapping of equivalent
> variables:
>
>   sg_bufp    => sg_buf_offset
>   sg_buf_end => sg_buf_end_offset
>   offp       => buffer_offset
>   off_start  => off_start_offset
>   off_end    => off_end_offset
>
> to construct an equivalent patch for this CVE without pulling in the
> other larger patch to base this on.
>
> No new compiler warnings for the android driver.
>
> @@ -3239,7 +3239,8 @@ static void binder_transaction(struct binder_proc *proc,      
>     buffer_offset = off_start_offset;                                              
>     off_end_offset = off_start_offset + tr->offsets_size;                          
>     sg_buf_offset = ALIGN(off_end_offset, sizeof(void *));                          
> -   sg_buf_end_offset = sg_buf_offset + extra_buffers_size;                        
> +   sg_buf_end_offset = sg_buf_offset + extra_buffers_size -                        
> +       ALIGN(secctx_sz, sizeof(u64));                                              
>     off_min = 0;                                                                    
>     for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;          
>          buffer_offset += sizeof(binder_size_t)) {
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [Disco][SRU][CVE-2019-2214] Fix for Binder OOB write

Kleber Souza
In reply to this post by Connor Kuehl
On 26.11.19 23:42, Connor Kuehl wrote:

> https://people.canonical.com/~ubuntu-security/cve/2019/CVE-2019-2214.html
>
> From the link above:
>
>     "In binder_transaction of binder.c, there is a possible out of bounds write
>     due to a missing bounds check. This could lead to local escalation of
>     privilege with no additional execution privileges needed. User interaction
>     is not needed for exploitation.Product: AndroidVersions: Android
>     kernelAndroid ID: A-136210786References: Upstream kernel"
>
> These backport notes are also included on the patch:
>
> There is a much larger cleanup patch (that Disco does not have) that moves
> away from using pointers and pointer arithmetic to using uintptr values,
> that patch is bde4a19fc04f ("binder: use userspace pointer as base of
> buffer space"). I used that patch to create a mapping of equivalent
> variables:
>
>   sg_bufp    => sg_buf_offset
>   sg_buf_end => sg_buf_end_offset
>   offp       => buffer_offset
>   off_start  => off_start_offset
>   off_end    => off_end_offset
>
> to construct an equivalent patch for this CVE without pulling in the
> other larger patch to base this on.
>
> No new compiler warnings for the android driver.
>
> @@ -3239,7 +3239,8 @@ static void binder_transaction(struct binder_proc *proc,      
>     buffer_offset = off_start_offset;                                              
>     off_end_offset = off_start_offset + tr->offsets_size;                          
>     sg_buf_offset = ALIGN(off_end_offset, sizeof(void *));                          
> -   sg_buf_end_offset = sg_buf_offset + extra_buffers_size;                        
> +   sg_buf_end_offset = sg_buf_offset + extra_buffers_size -                        
> +       ALIGN(secctx_sz, sizeof(u64));                                              
>     off_min = 0;                                                                    
>     for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;          
>          buffer_offset += sizeof(binder_size_t)) {
>

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [Disco][SRU][CVE-2019-2214] Fix for Binder OOB write

Khaled Elmously
In reply to this post by Connor Kuehl
On 2019-11-26 14:42:30 , Connor Kuehl wrote:

> https://people.canonical.com/~ubuntu-security/cve/2019/CVE-2019-2214.html
>
> From the link above:
>
>     "In binder_transaction of binder.c, there is a possible out of bounds write
>     due to a missing bounds check. This could lead to local escalation of
>     privilege with no additional execution privileges needed. User interaction
>     is not needed for exploitation.Product: AndroidVersions: Android
>     kernelAndroid ID: A-136210786References: Upstream kernel"
>
> These backport notes are also included on the patch:
>
> There is a much larger cleanup patch (that Disco does not have) that moves
> away from using pointers and pointer arithmetic to using uintptr values,
> that patch is bde4a19fc04f ("binder: use userspace pointer as base of
> buffer space"). I used that patch to create a mapping of equivalent
> variables:
>
>   sg_bufp    => sg_buf_offset
>   sg_buf_end => sg_buf_end_offset
>   offp       => buffer_offset
>   off_start  => off_start_offset
>   off_end    => off_end_offset
>
> to construct an equivalent patch for this CVE without pulling in the
> other larger patch to base this on.
>
> No new compiler warnings for the android driver.
>
> @@ -3239,7 +3239,8 @@ static void binder_transaction(struct binder_proc *proc,      
>     buffer_offset = off_start_offset;                                              
>     off_end_offset = off_start_offset + tr->offsets_size;                          
>     sg_buf_offset = ALIGN(off_end_offset, sizeof(void *));                          
> -   sg_buf_end_offset = sg_buf_offset + extra_buffers_size;                        
> +   sg_buf_end_offset = sg_buf_offset + extra_buffers_size -                        
> +       ALIGN(secctx_sz, sizeof(u64));                                              
>     off_min = 0;                                                                    
>     for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;          
>          buffer_offset += sizeof(binder_size_t)) {
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team