[PATCH 0/3][T] CVE-2017-0794 - Privilege escalation in the SCSI driver

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

[PATCH 0/3][T] CVE-2017-0794 - Privilege escalation in the SCSI driver

Tyler Hicks-2
https://people.canonical.com/~ubuntu-security/cve/2017/CVE-2017-0794.html

  A elevation of privilege vulnerability in the Upstream kernel scsi driver.

I've manually tested these patches using the Android reproducer[1] and
sg_dd from sg3-utils inside of a VM with a SCSI drive attached.

Note, this is a corrected patch set from when I used the wrong CVE ID[2] when
referring to this patch set. I've adjusted the CVE ID used in the commit
messages and tested with the correct reproducer for CVE-2017-0794.

[1] https://android.googlesource.com/platform/cts/+/0e7c60f79a319b4a4a3c5eee977ad912524da435/hostsidetests/security/securityPatch/Bug-35644812/poc.c
[2] https://lists.ubuntu.com/archives/kernel-team/2018-September/095470.html

Tyler



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

[PATCH 1/3] scsi: sg: protect accesses to 'reserved' page array

Tyler Hicks-2
From: Hannes Reinecke <[hidden email]>

The 'reserved' page array is used as a short-cut for mapping data,
saving us to allocate pages per request. However, the 'reserved' array
is only capable of holding one request, so this patch introduces a mutex
for protect 'sg_fd' against concurrent accesses.

Signed-off-by: Hannes Reinecke <[hidden email]>
Reviewed-by: Johannes Thumshirn <[hidden email]>
Tested-by: Johannes Thumshirn <[hidden email]>
Reviewed-by: Christoph Hellwig <[hidden email]>
Signed-off-by: Martin K. Petersen <[hidden email]>

CVE-2017-0794

(backported from commit 1bc0eb0446158cc76562176b80623aa119afee5b)
[tyhicks: Minor context changes]
Signed-off-by: Tyler Hicks <[hidden email]>
---
 drivers/scsi/sg.c | 53 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 4fecbb164104..ee23298dd955 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -151,6 +151,7 @@ typedef struct sg_fd { /* holds the state of a file descriptor */
  struct sg_device *parentdp; /* owning device */
  wait_queue_head_t read_wait; /* queue read until command done */
  rwlock_t rq_list_lock; /* protect access to list in req_arr */
+ struct mutex f_mutex; /* protect against changes in this fd */
  int timeout; /* defaults to SG_DEFAULT_TIMEOUT      */
  int timeout_user; /* defaults to SG_DEFAULT_TIMEOUT_USER */
  Sg_scatter_hold reserve; /* buffer held for this file descriptor */
@@ -164,6 +165,7 @@ typedef struct sg_fd { /* holds the state of a file descriptor */
  char next_cmd_len; /* 0 -> automatic (def), >0 -> use on next write() */
  char keep_orphan; /* 0 -> drop orphan (def), 1 -> keep for read() */
  char mmap_called; /* 0 -> mmap() never called on this fd */
+ char res_in_use; /* 1 -> 'reserve' array in use */
  struct kref f_ref;
  struct execute_work ew;
 } Sg_fd;
@@ -206,7 +208,6 @@ static void sg_remove_sfp(struct kref *);
 static Sg_request *sg_get_rq_mark(Sg_fd * sfp, int pack_id);
 static Sg_request *sg_add_request(Sg_fd * sfp);
 static int sg_remove_request(Sg_fd * sfp, Sg_request * srp);
-static int sg_res_in_use(Sg_fd * sfp);
 static Sg_device *sg_get_dev(int dev);
 static void sg_put_dev(Sg_device *sdp);
 
@@ -600,6 +601,7 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
  }
  buf += SZ_SG_HEADER;
  __get_user(opcode, buf);
+ mutex_lock(&sfp->f_mutex);
  if (sfp->next_cmd_len > 0) {
  if (sfp->next_cmd_len > MAX_COMMAND_SIZE) {
  SCSI_LOG_TIMEOUT(1, printk("sg_write: command length too long\n"));
@@ -614,6 +616,7 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
  if ((opcode >= 0xc0) && old_hdr.twelve_byte)
  cmd_size = 12;
  }
+ mutex_unlock(&sfp->f_mutex);
  SCSI_LOG_TIMEOUT(4, printk(
  "sg_write:   scsi opcode=0x%02x, cmd_size=%d\n", (int) opcode, cmd_size));
 /* Determine buffer size.  */
@@ -712,7 +715,7 @@ sg_new_write(Sg_fd *sfp, struct file *file, const char __user *buf,
  sg_remove_request(sfp, srp);
  return -EINVAL; /* either MMAP_IO or DIRECT_IO (not both) */
  }
- if (sg_res_in_use(sfp)) {
+ if (sfp->res_in_use) {
  sg_remove_request(sfp, srp);
  return -EBUSY; /* reserve buffer already being used */
  }
@@ -878,18 +881,22 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
  result = get_user(val, ip);
  if (result)
  return result;
+ mutex_lock(&sfp->f_mutex);
  if (val) {
  sfp->low_dma = 1;
- if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
+ if ((0 == sfp->low_dma) && (0 == sfp->res_in_use)) {
  val = (int) sfp->reserve.bufflen;
  sg_remove_scat(&sfp->reserve);
  sg_build_reserve(sfp, val);
  }
  } else {
- if (sdp->detached)
+ if (sdp->detached) {
+ mutex_unlock(&sfp->f_mutex);
  return -ENODEV;
+ }
  sfp->low_dma = sdp->device->host->unchecked_isa_dma;
  }
+ mutex_unlock(&sfp->f_mutex);
  return 0;
  case SG_GET_LOW_DMA:
  return put_user((int) sfp->low_dma, ip);
@@ -955,12 +962,18 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
                         return -EINVAL;
  val = min_t(int, val,
     queue_max_sectors(sdp->device->request_queue) * 512);
+ mutex_lock(&sfp->f_mutex);
  if (val != sfp->reserve.bufflen) {
- if (sg_res_in_use(sfp) || sfp->mmap_called)
+ if (sfp->mmap_called ||
+    sfp->res_in_use) {
+ mutex_unlock(&sfp->f_mutex);
  return -EBUSY;
+ }
+
  sg_remove_scat(&sfp->reserve);
  sg_build_reserve(sfp, val);
  }
+ mutex_unlock(&sfp->f_mutex);
  return 0;
  case SG_GET_RESERVED_SIZE:
  val = min_t(int, sfp->reserve.bufflen,
@@ -1685,13 +1698,22 @@ static int sg_start_req(Sg_request *srp, unsigned char *cmd)
  md = &map_data;
 
  if (md) {
- if (!sg_res_in_use(sfp) && dxfer_len <= rsv_schp->bufflen)
+ mutex_lock(&sfp->f_mutex);
+ if (dxfer_len <= rsv_schp->bufflen &&
+    !sfp->res_in_use) {
+ sfp->res_in_use = 1;
  sg_link_reserve(sfp, srp, dxfer_len);
- else {
+ } else if ((hp->flags & SG_FLAG_MMAP_IO) && sfp->res_in_use) {
+ mutex_unlock(&sfp->f_mutex);
+ return -EBUSY;
+ } else {
  res = sg_build_indirect(req_schp, sfp, dxfer_len);
- if (res)
+ if (res) {
+ mutex_unlock(&sfp->f_mutex);
  return res;
+ }
  }
+ mutex_unlock(&sfp->f_mutex);
 
  md->pages = req_schp->pages;
  md->page_order = req_schp->page_order;
@@ -2083,6 +2105,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
  rwlock_init(&sfp->rq_list_lock);
 
  kref_init(&sfp->f_ref);
+ mutex_init(&sfp->f_mutex);
  sfp->timeout = SG_DEFAULT_TIMEOUT;
  sfp->timeout_user = SG_DEFAULT_TIMEOUT_USER;
  sfp->force_packid = SG_DEF_FORCE_PACK_ID;
@@ -2152,20 +2175,6 @@ static void sg_remove_sfp(struct kref *kref)
  schedule_work(&sfp->ew.work);
 }
 
-static int
-sg_res_in_use(Sg_fd * sfp)
-{
- const Sg_request *srp;
- unsigned long iflags;
-
- read_lock_irqsave(&sfp->rq_list_lock, iflags);
- for (srp = sfp->headrp; srp; srp = srp->nextrp)
- if (srp->res_used)
- break;
- read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
- return srp ? 1 : 0;
-}
-
 #ifdef CONFIG_SCSI_PROC_FS
 static int
 sg_idr_max_id(int id, void *p, void *data)
--
2.7.4


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

[PATCH 2/3] scsi: sg: reset 'res_in_use' after unlinking reserved array

Tyler Hicks-2
In reply to this post by Tyler Hicks-2
From: Hannes Reinecke <[hidden email]>

Once the reserved page array is unused we can reset the 'res_in_use'
state; here we can do a lazy update without holding the mutex as we only
need to check against concurrent access, not concurrent release.

[mkp: checkpatch]

Fixes: 1bc0eb044615 ("scsi: sg: protect accesses to 'reserved' page array")
Signed-off-by: Hannes Reinecke <[hidden email]>
Reviewed-by: Johannes Thumshirn <[hidden email]>
Reviewed-by: Christoph Hellwig <[hidden email]>
Signed-off-by: Martin K. Petersen <[hidden email]>

CVE-2017-0794

(cherry picked from commit e791ce27c3f6a1d3c746fd6a8f8e36c9540ec6f9)
Signed-off-by: Tyler Hicks <[hidden email]>
---
 drivers/scsi/sg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index ee23298dd955..42e5a140ada5 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1994,6 +1994,8 @@ sg_unlink_reserve(Sg_fd * sfp, Sg_request * srp)
  req_schp->sglist_len = 0;
  sfp->save_scat_len = 0;
  srp->res_used = 0;
+ /* Called without mutex lock to avoid deadlock */
+ sfp->res_in_use = 0;
 }
 
 static Sg_request *
--
2.7.4


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

[PATCH 3/3] scsi: sg: recheck MMAP_IO request length with lock held

Tyler Hicks-2
In reply to this post by Tyler Hicks-2
From: Todd Poynor <[hidden email]>

Commit 1bc0eb044615 ("scsi: sg: protect accesses to 'reserved' page
array") adds needed concurrency protection for the "reserve" buffer.
Some checks that are initially made outside the lock are replicated once
the lock is taken to ensure the checks and resulting decisions are made
using consistent state.

The check that a request with flag SG_FLAG_MMAP_IO set fits in the
reserve buffer also needs to be performed again under the lock to ensure
the reserve buffer length compared against matches the value in effect
when the request is linked to the reserve buffer.  An -ENOMEM should be
returned in this case, instead of switching over to an indirect buffer
as for non-MMAP_IO requests.

Signed-off-by: Todd Poynor <[hidden email]>
Acked-by: Douglas Gilbert <[hidden email]>
Signed-off-by: Martin K. Petersen <[hidden email]>

CVE-2017-0794

(cherry picked from commit 8d26f491116feaa0b16de370b6a7ba40a40fa0b4)
Signed-off-by: Tyler Hicks <[hidden email]>
---
 drivers/scsi/sg.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 42e5a140ada5..7b99026238eb 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1703,9 +1703,12 @@ static int sg_start_req(Sg_request *srp, unsigned char *cmd)
     !sfp->res_in_use) {
  sfp->res_in_use = 1;
  sg_link_reserve(sfp, srp, dxfer_len);
- } else if ((hp->flags & SG_FLAG_MMAP_IO) && sfp->res_in_use) {
+ } else if (hp->flags & SG_FLAG_MMAP_IO) {
+ res = -EBUSY; /* sfp->res_in_use == 1 */
+ if (dxfer_len > rsv_schp->bufflen)
+ res = -ENOMEM;
  mutex_unlock(&sfp->f_mutex);
- return -EBUSY;
+ return res;
  } else {
  res = sg_build_indirect(req_schp, sfp, dxfer_len);
  if (res) {
--
2.7.4


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

ACK: [PATCH 0/3][T] CVE-2017-0794 - Privilege escalation in the SCSI driver

Stefan Bader-2
In reply to this post by Tyler Hicks-2
On 28.09.2018 23:29, Tyler Hicks wrote:

> https://people.canonical.com/~ubuntu-security/cve/2017/CVE-2017-0794.html
>
>   A elevation of privilege vulnerability in the Upstream kernel scsi driver.
>
> I've manually tested these patches using the Android reproducer[1] and
> sg_dd from sg3-utils inside of a VM with a SCSI drive attached.
>
> Note, this is a corrected patch set from when I used the wrong CVE ID[2] when
> referring to this patch set. I've adjusted the CVE ID used in the commit
> messages and tested with the correct reproducer for CVE-2017-0794.
>
> [1] https://android.googlesource.com/platform/cts/+/0e7c60f79a319b4a4a3c5eee977ad912524da435/hostsidetests/security/securityPatch/Bug-35644812/poc.c
> [2] https://lists.ubuntu.com/archives/kernel-team/2018-September/095470.html
>
> Tyler
>
>
>
Same patches before but ammended CVE number.

Acked-by: Stefan Bader <[hidden email]>


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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH 0/3][T] CVE-2017-0794 - Privilege escalation in the SCSI driver

Kleber Souza
In reply to this post by Tyler Hicks-2
On 09/28/18 23:29, Tyler Hicks wrote:

> https://people.canonical.com/~ubuntu-security/cve/2017/CVE-2017-0794.html
>
>   A elevation of privilege vulnerability in the Upstream kernel scsi driver.
>
> I've manually tested these patches using the Android reproducer[1] and
> sg_dd from sg3-utils inside of a VM with a SCSI drive attached.
>
> Note, this is a corrected patch set from when I used the wrong CVE ID[2] when
> referring to this patch set. I've adjusted the CVE ID used in the commit
> messages and tested with the correct reproducer for CVE-2017-0794.
>
> [1] https://android.googlesource.com/platform/cts/+/0e7c60f79a319b4a4a3c5eee977ad912524da435/hostsidetests/security/securityPatch/Bug-35644812/poc.c
> [2] https://lists.ubuntu.com/archives/kernel-team/2018-September/095470.html
>
> Tyler
>
>
>

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: [PATCH 0/3][T] CVE-2017-0794 - Privilege escalation in the SCSI driver

Stefan Bader-2
In reply to this post by Tyler Hicks-2
On 28.09.2018 23:29, Tyler Hicks wrote:

> https://people.canonical.com/~ubuntu-security/cve/2017/CVE-2017-0794.html
>
>   A elevation of privilege vulnerability in the Upstream kernel scsi driver.
>
> I've manually tested these patches using the Android reproducer[1] and
> sg_dd from sg3-utils inside of a VM with a SCSI drive attached.
>
> Note, this is a corrected patch set from when I used the wrong CVE ID[2] when
> referring to this patch set. I've adjusted the CVE ID used in the commit
> messages and tested with the correct reproducer for CVE-2017-0794.
>
> [1] https://android.googlesource.com/platform/cts/+/0e7c60f79a319b4a4a3c5eee977ad912524da435/hostsidetests/security/securityPatch/Bug-35644812/poc.c
> [2] https://lists.ubuntu.com/archives/kernel-team/2018-September/095470.html
>
> Tyler
>
>
>
Applied to trusty/master-next. Thanks.

-Stefan


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

signature.asc (836 bytes) Download Attachment