Skip to contentSkip to conversation

sg: fix infoleak when using SG_GET_REQUEST_TABLE

PATCH Applied Hannes Reinecke 7 participants 13 messages Sep 15, 2017

Subsystems (2)
SCSI SCSI SG driver
lore
Share & tools
lore

Hannes Reinecke

author
#0 lore raw

cover sg: fix infoleak when using SG_GET_REQUEST_TABLE

KMSAN detected a possible infoleak when using SG_GET_REQUEST_TABLE ioctl. This patchset fixes it.

As usual, comments and reviews are welcome.

Hannes Reinecke (2):
sg: factor out sg_fill_request_table()
sg: fixup infoleak when using SG_GET_REQUEST_TABLE

 drivers/scsi/sg.c | 65 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 28 deletions(-)
Signature · 1 line
1.8.5.6
author
#1 lore raw

PATCH 1/2 sg: factor out sg_fill_request_table()

Factor our sg_fill_request_table() for better readability.

Signed-off-by Hannes Reinecke <[email protected]>
---
 drivers/scsi/sg.c | 62 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 26 deletions(-)
drivers/scsi/sg.c +36 -26
index 03194c4..f1e20ca0 100644
@@ -857,6 +857,40 @@ static int max_sectors_bytes(struct request_queue *q)
 	return max_sectors << 9;
 }
 
+static void
+sg_fill_request_table(Sg_fd *sfp, sg_req_info_t *rinfo)
+{
+	Sg_request *srp;
+	int val;
+	unsigned int ms;
+
+	val = 0;
+	list_for_each_entry(srp, &sfp->rq_list, entry) {
+		if (val > SG_MAX_QUEUE)
+			break;
+		memset(&rinfo[val], 0, SZ_SG_REQ_INFO);
+		rinfo[val].req_state = srp->done + 1;
+		rinfo[val].problem =
+			srp->header.masked_status &
+			srp->header.host_status &
+			srp->header.driver_status;
+		if (srp->done)
+			rinfo[val].duration =
+				srp->header.duration;
+		else {
+			ms = jiffies_to_msecs(jiffies);
+			rinfo[val].duration =
+				(ms > srp->header.duration) ?
+				(ms - srp->header.duration) : 0;
+		}
+		rinfo[val].orphan = srp->orphan;
+		rinfo[val].sg_io_owned = srp->sg_io_owned;
+		rinfo[val].pack_id = srp->header.pack_id;
+		rinfo[val].usr_ptr = srp->header.usr_ptr;
+		val++;
+	}
+}
+
 static long
 sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 {

@@ -1041,39 +1075,15 @@ static int max_sectors_bytes(struct request_queue *q)
 			return -EFAULT;
 		else {
 			sg_req_info_t *rinfo;
-			unsigned int ms;
 
 			rinfo = kmalloc(SZ_SG_REQ_INFO * SG_MAX_QUEUE,
 								GFP_KERNEL);
 			if (!rinfo)
 				return -ENOMEM;
 			read_lock_irqsave(&sfp->rq_list_lock, iflags);
-			val = 0;
-			list_for_each_entry(srp, &sfp->rq_list, entry) {
-				if (val > SG_MAX_QUEUE)
-					break;
-				memset(&rinfo[val], 0, SZ_SG_REQ_INFO);
-				rinfo[val].req_state = srp->done + 1;
-				rinfo[val].problem =
-					srp->header.masked_status &
-					srp->header.host_status &
-					srp->header.driver_status;
-				if (srp->done)
-					rinfo[val].duration =
-						srp->header.duration;
-				else {
-					ms = jiffies_to_msecs(jiffies);
-					rinfo[val].duration =
-						(ms > srp->header.duration) ?
-						(ms - srp->header.duration) : 0;
-				}
-				rinfo[val].orphan = srp->orphan;
-				rinfo[val].sg_io_owned = srp->sg_io_owned;
-				rinfo[val].pack_id = srp->header.pack_id;
-				rinfo[val].usr_ptr = srp->header.usr_ptr;
-				val++;
-			}
+			sg_fill_request_table(sfp, rinfo);
 			read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
+
 			result = __copy_to_user(p, rinfo,
 						SZ_SG_REQ_INFO * SG_MAX_QUEUE);
 			result = result ? -EFAULT : 0;
Signature · 1 line
1.8.5.6
#4 lore raw

Re: [PATCH 1/2] sg: factor out sg_fill_request_table()

In reply to Hannes Reinecke

Hannes,

Quoted 1 line

Factor our sg_fill_request_table() for better readability.

         ^^^ out

Fixed typo and applied to 4.14/scsi-fixes. I had to do it by hand so please check.

Thanks!

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
maintainer Acked-by on v1 1/2
#5 lore raw

Re: [PATCH 1/2] sg: factor out sg_fill_request_table()

In reply to Hannes Reinecke
Quoted 3 lines · Hannes Reinecke

Factor our sg_fill_request_table() for better readability.

Signed-off-by Hannes Reinecke <[email protected]>
Acked-by Douglas Gilbert <[email protected]>

Thanks.

Quoted 92 lines
---
  drivers/scsi/sg.c | 62 ++++++++++++++++++++++++++++++++-----------------------
  1 file changed, 36 insertions(+), 26 deletions(-)
drivers/scsi/sg.c +36 -26
index 03194c4..f1e20ca0 100644
@@ -857,6 +857,40 @@ static int max_sectors_bytes(struct request_queue *q)
  	return max_sectors << 9;
  }
 
+static void
+sg_fill_request_table(Sg_fd *sfp, sg_req_info_t *rinfo)
+{
+	Sg_request *srp;
+	int val;
+	unsigned int ms;
+
+	val = 0;
+	list_for_each_entry(srp, &sfp->rq_list, entry) {
+		if (val > SG_MAX_QUEUE)
+			break;
+		memset(&rinfo[val], 0, SZ_SG_REQ_INFO);
+		rinfo[val].req_state = srp->done + 1;
+		rinfo[val].problem =
+			srp->header.masked_status &
+			srp->header.host_status &
+			srp->header.driver_status;
+		if (srp->done)
+			rinfo[val].duration =
+				srp->header.duration;
+		else {
+			ms = jiffies_to_msecs(jiffies);
+			rinfo[val].duration =
+				(ms > srp->header.duration) ?
+				(ms - srp->header.duration) : 0;
+		}
+		rinfo[val].orphan = srp->orphan;
+		rinfo[val].sg_io_owned = srp->sg_io_owned;
+		rinfo[val].pack_id = srp->header.pack_id;
+		rinfo[val].usr_ptr = srp->header.usr_ptr;
+		val++;
+	}
+}
+
  static long
  sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
  {

@@ -1041,39 +1075,15 @@ static int max_sectors_bytes(struct request_queue *q)
  			return -EFAULT;
  		else {
  			sg_req_info_t *rinfo;
-			unsigned int ms;
 
  			rinfo = kmalloc(SZ_SG_REQ_INFO * SG_MAX_QUEUE,
  								GFP_KERNEL);
  			if (!rinfo)
  				return -ENOMEM;
  			read_lock_irqsave(&sfp->rq_list_lock, iflags);
-			val = 0;
-			list_for_each_entry(srp, &sfp->rq_list, entry) {
-				if (val > SG_MAX_QUEUE)
-					break;
-				memset(&rinfo[val], 0, SZ_SG_REQ_INFO);
-				rinfo[val].req_state = srp->done + 1;
-				rinfo[val].problem =
-					srp->header.masked_status &
-					srp->header.host_status &
-					srp->header.driver_status;
-				if (srp->done)
-					rinfo[val].duration =
-						srp->header.duration;
-				else {
-					ms = jiffies_to_msecs(jiffies);
-					rinfo[val].duration =
-						(ms > srp->header.duration) ?
-						(ms - srp->header.duration) : 0;
-				}
-				rinfo[val].orphan = srp->orphan;
-				rinfo[val].sg_io_owned = srp->sg_io_owned;
-				rinfo[val].pack_id = srp->header.pack_id;
-				rinfo[val].usr_ptr = srp->header.usr_ptr;
-				val++;
-			}
+			sg_fill_request_table(sfp, rinfo);
  			read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
+
  			result = __copy_to_user(p, rinfo,
  						SZ_SG_REQ_INFO * SG_MAX_QUEUE);
  			result = result ? -EFAULT : 0;
author
#6 lore raw

PATCH 2/2 sg: fixup infoleak when using SG_GET_REQUEST_TABLE

In reply to Hannes Reinecke

When calling SG_GET_REQUEST_TABLE ioctl that only a half-filled table is returned; the remaining part will then contain stale kernel memory information.
This patch zeroes out the entire table to avoid this issue.

Signed-off-by Hannes Reinecke <[email protected]>
---
 drivers/scsi/sg.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/scsi/sg.c +2 -3
index f1e20ca0..7f98ab4 100644
@@ -868,7 +868,6 @@ static int max_sectors_bytes(struct request_queue *q)
 	list_for_each_entry(srp, &sfp->rq_list, entry) {
 		if (val > SG_MAX_QUEUE)
 			break;
-		memset(&rinfo[val], 0, SZ_SG_REQ_INFO);
 		rinfo[val].req_state = srp->done + 1;
 		rinfo[val].problem =
 			srp->header.masked_status &

@@ -1076,8 +1075,8 @@ static int max_sectors_bytes(struct request_queue *q)
 		else {
 			sg_req_info_t *rinfo;
 
-			rinfo = kmalloc(SZ_SG_REQ_INFO * SG_MAX_QUEUE,
-								GFP_KERNEL);
+			rinfo = kzalloc(SZ_SG_REQ_INFO * SG_MAX_QUEUE,
+					GFP_KERNEL);
 			if (!rinfo)
 				return -ENOMEM;
 			read_lock_irqsave(&sfp->rq_list_lock, iflags);
Signature · 1 line
1.8.5.6
Reviewed-by on v1 2/2
#7 lore raw

Re: [PATCH 2/2] sg: fixup infoleak when using SG_GET_REQUEST_TABLE

Quoted 4 lines · Hannes Reinecke

When calling SG_GET_REQUEST_TABLE ioctl that only a half-filled table is returned; the remaining part will then contain stale kernel memory information.
This patch zeroes out the entire table to avoid this issue.

Reviewed-by Bart Van Assche <[email protected]>
Reviewed-by on v1 2/2
#8 lore raw

Re: [PATCH 2/2] sg: fixup infoleak when using SG_GET_REQUEST_TABLE

In reply to Hannes Reinecke
Quoted 6 lines · Hannes Reinecke

When calling SG_GET_REQUEST_TABLE ioctl that only a half-filled table is returned; the remaining part will then contain stale kernel memory information.
This patch zeroes out the entire table to avoid this issue.

Signed-off-by Hannes Reinecke <[email protected]>

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>
Reviewed-by on v1 2/2
#9 lore raw

Re: [PATCH 2/2] sg: fixup infoleak when using SG_GET_REQUEST_TABLE

In reply to Hannes Reinecke
Quoted 9 lines · Hannes Reinecke

When calling SG_GET_REQUEST_TABLE ioctl that only a half-filled table is returned; the remaining part will then contain stale kernel memory information.
This patch zeroes out the entire table to avoid this issue.

Signed-off-by Hannes Reinecke <[email protected]>
---
 drivers/scsi/sg.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by Eric Dumazet <[email protected]>

Thanks !

#10 lore raw

Re: [PATCH 2/2] sg: fixup infoleak when using SG_GET_REQUEST_TABLE

In reply to Hannes Reinecke

Looks good.

James, I expect I'll be getting this through the normal SCSI pulls..

Linus

Quoted 4 lines · Hannes Reinecke

When calling SG_GET_REQUEST_TABLE ioctl that only a half-filled table is returned; the remaining part will then contain stale kernel memory information.
This patch zeroes out the entire table to avoid this issue.

Applied
#11 lore raw

Re: [PATCH 2/2] sg: fixup infoleak when using SG_GET_REQUEST_TABLE

In reply to Hannes Reinecke

Hannes,

Quoted 4 lines

When calling SG_GET_REQUEST_TABLE ioctl that only a half-filled table is returned; the remaining part will then contain stale kernel memory information. This patch zeroes out the entire table to avoid this issue.

Applied to 4.14/scsi-fixes. Thank you!

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
maintainer Acked-by on v1 2/2
#12 lore raw

Re: [PATCH 2/2] sg: fixup infoleak when using SG_GET_REQUEST_TABLE

In reply to Hannes Reinecke
Quoted 6 lines · Hannes Reinecke

When calling SG_GET_REQUEST_TABLE ioctl that only a half-filled table is returned; the remaining part will then contain stale kernel memory information.
This patch zeroes out the entire table to avoid this issue.

Signed-off-by Hannes Reinecke <[email protected]>
Acked-by Douglas Gilbert <[email protected]>

Thanks.

Quoted 28 lines
---
  drivers/scsi/sg.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)
drivers/scsi/sg.c +2 -3
index f1e20ca0..7f98ab4 100644
@@ -868,7 +868,6 @@ static int max_sectors_bytes(struct request_queue *q)
  	list_for_each_entry(srp, &sfp->rq_list, entry) {
  		if (val > SG_MAX_QUEUE)
  			break;
-		memset(&rinfo[val], 0, SZ_SG_REQ_INFO);
  		rinfo[val].req_state = srp->done + 1;
  		rinfo[val].problem =
  			srp->header.masked_status &

@@ -1076,8 +1075,8 @@ static int max_sectors_bytes(struct request_queue *q)
  		else {
  			sg_req_info_t *rinfo;
 
-			rinfo = kmalloc(SZ_SG_REQ_INFO * SG_MAX_QUEUE,
-								GFP_KERNEL);
+			rinfo = kzalloc(SZ_SG_REQ_INFO * SG_MAX_QUEUE,
+					GFP_KERNEL);
  			if (!rinfo)
  				return -ENOMEM;
  			read_lock_irqsave(&sfp->rq_list_lock, iflags);