Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1448066 > unrolled thread

[PATCH resend 2/5] libata-scsi: fix read-only bits checking in ata_mselect_*()

Started bytom.ty89@gmail.com
First post2016-07-21 20:50 +0200
Last post2016-07-22 01:40 +0200
Articles 14 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH resend 2/5] libata-scsi: fix read-only bits checking in ata_mselect_*() tom.ty89@gmail.com - 2016-07-21 20:50 +0200
    [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy tom.ty89@gmail.com - 2016-07-21 20:50 +0200
      [PATCH resend 4/5] libata-scsi: have all checks done before calling ata_mselect_*() tom.ty89@gmail.com - 2016-07-21 20:50 +0200
      Re: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy Tejun Heo <tj@kernel.org> - 2016-07-21 23:20 +0200
        Re: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy Tom Yan <tom.ty89@gmail.com> - 2016-07-21 23:40 +0200
          Re: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy Tejun Heo <tj@kernel.org> - 2016-07-21 23:50 +0200
            [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy tom.ty89@gmail.com - 2016-07-22 01:30 +0200
              Re: [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode  page copy Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-07-22 12:00 +0200
                Re: [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode  page copy Tom Yan <tom.ty89@gmail.com> - 2016-07-22 20:30 +0200
                  [PATCH resend v3 3/5] libata-scsi: use u8 array to store mode page copy tom.ty89@gmail.com - 2016-07-22 20:40 +0200
            [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy tom.ty89@gmail.com - 2016-07-22 01:40 +0200
    Re: [PATCH resend 2/5] libata-scsi: fix read-only bits checking in ata_mselect_*() Tom Yan <tom.ty89@gmail.com> - 2016-07-22 01:30 +0200
      [PATCH resend v2 2/5] libata-scsi: fix read-only bits checking in ata_mselect_*() tom.ty89@gmail.com - 2016-07-22 01:30 +0200
      [PATCH resend v2 2/5] libata-scsi: fix read-only bits checking in ata_mselect_*() tom.ty89@gmail.com - 2016-07-22 01:40 +0200

#1448066 — [PATCH resend 2/5] libata-scsi: fix read-only bits checking in ata_mselect_*()

Fromtom.ty89@gmail.com
Date2016-07-21 20:50 +0200
Subject[PATCH resend 2/5] libata-scsi: fix read-only bits checking in ata_mselect_*()
Message-ID<rXrcJ-2T6-7@gated-at.bofh.it>
From: Tom Yan <tom.ty89@gmail.com>

Commit 7780081c1f04 ("libata-scsi: Set information sense field for
invalid parameter") changed how ata_mselect_*() make sure read-only
bits are not modified. The new implementation introduced a bug that
the read-only bits in the byte that has a changeable bit will not
be checked.

Added the necessary check, with comments explaining the heuristic.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index eb5e8ff..ac90676 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3631,8 +3631,18 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	 */
 	ata_msense_caching(dev->id, mpage, false);
 	for (i = 0; i < CACHE_MPAGE_LEN - 2; i++) {
-		if (i == 0)
-			continue;
+		/* Check the first byte */
+		if (i == 0) {
+			/* except the WCE bit */
+			if ((mpage[i + 2] & 0xfb) != (buf[i] & 0xfb)) {
+				*fp = i;
+				return -EINVAL;
+			} else {
+				continue;
+			}
+		}
+
+		/* Check the remaining bytes */
 		if (mpage[i + 2] != buf[i]) {
 			*fp = i;
 			return -EINVAL;
@@ -3686,8 +3696,18 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 	 */
 	ata_msense_control(dev, mpage, false);
 	for (i = 0; i < CONTROL_MPAGE_LEN - 2; i++) {
-		if (i == 0)
-			continue;
+		/* Check the first byte */
+		if (i == 0) {
+			/* except the D_SENSE bit */
+			if ((mpage[i + 2] & 0xfb) != (buf[i] & 0xfb)) {
+				*fp = i;
+				return -EINVAL;
+			} else {
+				continue;
+			}
+		}
+
+		/* Check the remaining bytes */
 		if (mpage[2 + i] != buf[i]) {
 			*fp = i;
 			return -EINVAL;
-- 
2.9.0

[toc] | [next] | [standalone]

#1448068 — [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy

Fromtom.ty89@gmail.com
Date2016-07-21 20:50 +0200
Subject[PATCH resend 3/5] libata-scsi: fix overflow in mode page copy
Message-ID<rXrcJ-2T6-9@gated-at.bofh.it>
In reply to#1448066
From: Tom Yan <tom.ty89@gmail.com>

ata_mselect_*() would initialize a char array for storing a copy of
the current mode page. However, if char was actually signed char,
overflow could occur.

For example, `0xff` from def_control_mpage[] would be "truncated"
to `-1`. This prevented ata_mselect_control() from working at all,
since when it did the read-only bits check, there would always be
a mismatch.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index ac90676..3c93341 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 {
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
-	char mpage[CACHE_MPAGE_LEN];
+	u8 mpage[CACHE_MPAGE_LEN];
 	u8 wce;
 	int i;
 
@@ -3675,7 +3675,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 			       const u8 *buf, int len, u16 *fp)
 {
 	struct ata_device *dev = qc->dev;
-	char mpage[CONTROL_MPAGE_LEN];
+	u8 mpage[CONTROL_MPAGE_LEN];
 	u8 d_sense;
 	int i;
 
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1448070 — [PATCH resend 4/5] libata-scsi: have all checks done before calling ata_mselect_*()

Fromtom.ty89@gmail.com
Date2016-07-21 20:50 +0200
Subject[PATCH resend 4/5] libata-scsi: have all checks done before calling ata_mselect_*()
Message-ID<rXrcJ-2T6-11@gated-at.bofh.it>
In reply to#1448068
From: Tom Yan <tom.ty89@gmail.com>

The one-page-at-a-time check in ata_scsi_mode_select_xlat() should
be done before either of the ata_mselect_*() is called.

Also updated the comment. We have more than one mode page that has
changeable bit since commit 06dbde5f3a44 ("libata: Implement
control mode page to select sense format").

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 3c93341..6c424c5 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3837,6 +3837,12 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 	if (pg_len > len)
 		goto invalid_param_len;
 
+	/*
+	 * Currently we only support setting one page at a time.
+	 */
+	if (len > pg_len)
+		goto invalid_param;
+
 	switch (pg) {
 	case CACHE_MPAGE:
 		if (ata_mselect_caching(qc, p, pg_len, &fp) < 0) {
@@ -3855,13 +3861,6 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 		goto invalid_param;
 	}
 
-	/*
-	 * Only one page has changeable data, so we only support setting one
-	 * page at a time.
-	 */
-	if (len > pg_len)
-		goto invalid_param;
-
 	return 0;
 
  invalid_fld:
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1448150 — Re: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy

FromTejun Heo <tj@kernel.org>
Date2016-07-21 23:20 +0200
SubjectRe: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy
Message-ID<rXtxT-4A4-9@gated-at.bofh.it>
In reply to#1448068
Hello,

On Fri, Jul 22, 2016 at 02:41:52AM +0800, tom.ty89@gmail.com wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> ata_mselect_*() would initialize a char array for storing a copy of
> the current mode page. However, if char was actually signed char,
> overflow could occur.

Do you mean sign extension?

> For example, `0xff` from def_control_mpage[] would be "truncated"
> to `-1`. This prevented ata_mselect_control() from working at all,
> since when it did the read-only bits check, there would always be
> a mismatch.

Heh, the description doesn't really make sense.  Are you talking about
something like the following?

	char ar[N];
	int i;

	i = ar[x];
	if (i == 0xff)
		asdf;

If so, the description isn't quite right.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1448195 — Re: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy

FromTom Yan <tom.ty89@gmail.com>
Date2016-07-21 23:40 +0200
SubjectRe: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy
Message-ID<rXtRf-4HJ-11@gated-at.bofh.it>
In reply to#1448150
Well, I mean this is happening when ata_mselect_*() calls ata_msense_*():

[tom@localhost ~]$ cat test.c
#include <stdio.h>
#include <string.h>

typedef unsigned char u8;

int main() {
  u8 a[2] = { 0xff, 0xff };
  char b[2];
  memcpy(b, a, 2);

  for (int i=0; i<2; i++) {
    printf("%d\n", a[i]);
  }

  for (int i=0; i<2; i++) {
    printf("%d\n", b[i]);
  }
}

[tom@localhost ~]$ cc test.c

[tom@localhost ~]$ ./a.out
255
255
-1
-1

Let me know how I should polish the description for this.

On 22 July 2016 at 05:17, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Fri, Jul 22, 2016 at 02:41:52AM +0800, tom.ty89@gmail.com wrote:
>> From: Tom Yan <tom.ty89@gmail.com>
>>
>> ata_mselect_*() would initialize a char array for storing a copy of
>> the current mode page. However, if char was actually signed char,
>> overflow could occur.
>
> Do you mean sign extension?
>
>> For example, `0xff` from def_control_mpage[] would be "truncated"
>> to `-1`. This prevented ata_mselect_control() from working at all,
>> since when it did the read-only bits check, there would always be
>> a mismatch.
>
> Heh, the description doesn't really make sense.  Are you talking about
> something like the following?
>
>         char ar[N];
>         int i;
>
>         i = ar[x];
>         if (i == 0xff)
>                 asdf;
>
> If so, the description isn't quite right.
>
> Thanks.
>
> --
> tejun

[toc] | [prev] | [next] | [standalone]

#1448196 — Re: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy

FromTejun Heo <tj@kernel.org>
Date2016-07-21 23:50 +0200
SubjectRe: [PATCH resend 3/5] libata-scsi: fix overflow in mode page copy
Message-ID<rXu0V-4Lj-3@gated-at.bofh.it>
In reply to#1448195
On Fri, Jul 22, 2016 at 05:39:27AM +0800, Tom Yan wrote:
> Let me know how I should polish the description for this.

The above is because the signed ones are getting sign-extended making
them different from the unsigned ones which don't get padded in the
high bits.  Converting to u8 is the right thing to do but nothing here
is getting truncated.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1448247 — [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy

Fromtom.ty89@gmail.com
Date2016-07-22 01:30 +0200
Subject[PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy
Message-ID<rXvzI-5X0-1@gated-at.bofh.it>
In reply to#1448196
From: Tom Yan <tom.ty89@gmail.com>

ata_mselect_*() would initialize a char array for storing a copy of
the current mode page. However, char could be signed char. In that
case, bytes larger than 127 would be converted to negative number.

For example, 0xff from def_control_mpage[] would become -1. This
prevented ata_mselect_control() from working at all, since when it
did the read-only bits check, there would always be a mismatch.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4a4e6f1..a28e2ea94 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 {
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
-	char mpage[CACHE_MPAGE_LEN];
+	u8 mpage[CACHE_MPAGE_LEN];
 	u8 wce, mask;
 	int i;
 
@@ -3668,7 +3668,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 			       const u8 *buf, int len, u16 *fp)
 {
 	struct ata_device *dev = qc->dev;
-	char mpage[CONTROL_MPAGE_LEN];
+        u8 mpage[CONTROL_MPAGE_LEN];
 	u8 d_sense, mask;
 	int i;
 
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1448514 — Re: [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy

FromSergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date2016-07-22 12:00 +0200
SubjectRe: [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy
Message-ID<rXFpn-46k-11@gated-at.bofh.it>
In reply to#1448247
Hello.

On 7/22/2016 2:29 AM, tom.ty89@gmail.com wrote:

> From: Tom Yan <tom.ty89@gmail.com>
>
> ata_mselect_*() would initialize a char array for storing a copy of
> the current mode page. However, char could be signed char. In that
> case, bytes larger than 127 would be converted to negative number.
>
> For example, 0xff from def_control_mpage[] would become -1. This
> prevented ata_mselect_control() from working at all, since when it
> did the read-only bits check, there would always be a mismatch.
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 4a4e6f1..a28e2ea94 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
>  {
>  	struct ata_taskfile *tf = &qc->tf;
>  	struct ata_device *dev = qc->dev;
> -	char mpage[CACHE_MPAGE_LEN];
> +	u8 mpage[CACHE_MPAGE_LEN];
>  	u8 wce, mask;
>  	int i;
>
> @@ -3668,7 +3668,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
>  			       const u8 *buf, int len, u16 *fp)
>  {
>  	struct ata_device *dev = qc->dev;
> -	char mpage[CONTROL_MPAGE_LEN];
> +        u8 mpage[CONTROL_MPAGE_LEN];

    Indent with tabs please, as above.

[...]

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1448715 — Re: [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy

FromTom Yan <tom.ty89@gmail.com>
Date2016-07-22 20:30 +0200
SubjectRe: [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy
Message-ID<rXNmW-WX-15@gated-at.bofh.it>
In reply to#1448514
Strange. I merely changed the two "char" to "u8". I wonder how the tab
became spaces. Anyway, sorry about that, resending soon.

On 22 July 2016 at 17:59, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
>
> On 7/22/2016 2:29 AM, tom.ty89@gmail.com wrote:
>
>> From: Tom Yan <tom.ty89@gmail.com>
>>
>> ata_mselect_*() would initialize a char array for storing a copy of
>> the current mode page. However, char could be signed char. In that
>> case, bytes larger than 127 would be converted to negative number.
>>
>> For example, 0xff from def_control_mpage[] would become -1. This
>> prevented ata_mselect_control() from working at all, since when it
>> did the read-only bits check, there would always be a mismatch.
>>
>> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 4a4e6f1..a28e2ea94 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd
>> *qc,
>>  {
>>         struct ata_taskfile *tf = &qc->tf;
>>         struct ata_device *dev = qc->dev;
>> -       char mpage[CACHE_MPAGE_LEN];
>> +       u8 mpage[CACHE_MPAGE_LEN];
>>         u8 wce, mask;
>>         int i;
>>
>> @@ -3668,7 +3668,7 @@ static int ata_mselect_control(struct ata_queued_cmd
>> *qc,
>>                                const u8 *buf, int len, u16 *fp)
>>  {
>>         struct ata_device *dev = qc->dev;
>> -       char mpage[CONTROL_MPAGE_LEN];
>> +        u8 mpage[CONTROL_MPAGE_LEN];
>
>
>    Indent with tabs please, as above.
>
> [...]
>
> MBR, Sergei
>

[toc] | [prev] | [next] | [standalone]

#1448719 — [PATCH resend v3 3/5] libata-scsi: use u8 array to store mode page copy

Fromtom.ty89@gmail.com
Date2016-07-22 20:40 +0200
Subject[PATCH resend v3 3/5] libata-scsi: use u8 array to store mode page copy
Message-ID<rXNwB-117-3@gated-at.bofh.it>
In reply to#1448715
From: Tom Yan <tom.ty89@gmail.com>

ata_mselect_*() would initialize a char array for storing a copy of
the current mode page. However, char could be signed char. In that
case, bytes larger than 127 would be converted to negative number.

For example, 0xff from def_control_mpage[] would become -1. This
prevented ata_mselect_control() from working at all, since when it
did the read-only bits check, there would always be a mismatch.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4a4e6f1..5cfcb4b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 {
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
-	char mpage[CACHE_MPAGE_LEN];
+	u8 mpage[CACHE_MPAGE_LEN];
 	u8 wce, mask;
 	int i;
 
@@ -3668,7 +3668,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 			       const u8 *buf, int len, u16 *fp)
 {
 	struct ata_device *dev = qc->dev;
-	char mpage[CONTROL_MPAGE_LEN];
+	u8 mpage[CONTROL_MPAGE_LEN];
 	u8 d_sense, mask;
 	int i;
 
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1448256 — [PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy

Fromtom.ty89@gmail.com
Date2016-07-22 01:40 +0200
Subject[PATCH resend v2 3/5] libata-scsi: use u8 array to store mode page copy
Message-ID<rXvJn-60q-5@gated-at.bofh.it>
In reply to#1448196
From: Tom Yan <tom.ty89@gmail.com>

ata_mselect_*() would initialize a char array for storing a copy of
the current mode page. However, char could be signed char. In that
case, bytes larger than 127 would be converted to negative number.

For example, 0xff from def_control_mpage[] would become -1. This
prevented ata_mselect_control() from working at all, since when it
did the read-only bits check, there would always be a mismatch.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4a4e6f1..a28e2ea94 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3610,7 +3610,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 {
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
-	char mpage[CACHE_MPAGE_LEN];
+	u8 mpage[CACHE_MPAGE_LEN];
 	u8 wce, mask;
 	int i;
 
@@ -3668,7 +3668,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 			       const u8 *buf, int len, u16 *fp)
 {
 	struct ata_device *dev = qc->dev;
-	char mpage[CONTROL_MPAGE_LEN];
+        u8 mpage[CONTROL_MPAGE_LEN];
 	u8 d_sense, mask;
 	int i;
 
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1448249

FromTom Yan <tom.ty89@gmail.com>
Date2016-07-22 01:30 +0200
Message-ID<rXvzI-5X0-9@gated-at.bofh.it>
In reply to#1448066
This is actually a bit clumsy. Sending a rewritten version.

On 22 July 2016 at 02:41,  <tom.ty89@gmail.com> wrote:
> From: Tom Yan <tom.ty89@gmail.com>
>
> Commit 7780081c1f04 ("libata-scsi: Set information sense field for
> invalid parameter") changed how ata_mselect_*() make sure read-only
> bits are not modified. The new implementation introduced a bug that
> the read-only bits in the byte that has a changeable bit will not
> be checked.
>
> Added the necessary check, with comments explaining the heuristic.
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index eb5e8ff..ac90676 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3631,8 +3631,18 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
>          */
>         ata_msense_caching(dev->id, mpage, false);
>         for (i = 0; i < CACHE_MPAGE_LEN - 2; i++) {
> -               if (i == 0)
> -                       continue;
> +               /* Check the first byte */
> +               if (i == 0) {
> +                       /* except the WCE bit */
> +                       if ((mpage[i + 2] & 0xfb) != (buf[i] & 0xfb)) {
> +                               *fp = i;
> +                               return -EINVAL;
> +                       } else {
> +                               continue;
> +                       }
> +               }
> +
> +               /* Check the remaining bytes */
>                 if (mpage[i + 2] != buf[i]) {
>                         *fp = i;
>                         return -EINVAL;
> @@ -3686,8 +3696,18 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
>          */
>         ata_msense_control(dev, mpage, false);
>         for (i = 0; i < CONTROL_MPAGE_LEN - 2; i++) {
> -               if (i == 0)
> -                       continue;
> +               /* Check the first byte */
> +               if (i == 0) {
> +                       /* except the D_SENSE bit */
> +                       if ((mpage[i + 2] & 0xfb) != (buf[i] & 0xfb)) {
> +                               *fp = i;
> +                               return -EINVAL;
> +                       } else {
> +                               continue;
> +                       }
> +               }
> +
> +               /* Check the remaining bytes */
>                 if (mpage[2 + i] != buf[i]) {
>                         *fp = i;
>                         return -EINVAL;
> --
> 2.9.0
>

[toc] | [prev] | [next] | [standalone]

#1448253 — [PATCH resend v2 2/5] libata-scsi: fix read-only bits checking in ata_mselect_*()

Fromtom.ty89@gmail.com
Date2016-07-22 01:30 +0200
Subject[PATCH resend v2 2/5] libata-scsi: fix read-only bits checking in ata_mselect_*()
Message-ID<rXvzI-5X0-21@gated-at.bofh.it>
In reply to#1448249
From: Tom Yan <tom.ty89@gmail.com>

Commit 7780081c1f04 ("libata-scsi: Set information sense field for
invalid parameter") changed how ata_mselect_*() make sure read-only
bits are not modified. The new implementation introduced a bug that
the read-only bits in the byte that has a changeable bit will not
be checked.

Made it check every byte but mask the changeable bit.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index eb5e8ff..4a4e6f1 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3611,7 +3611,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
 	char mpage[CACHE_MPAGE_LEN];
-	u8 wce;
+	u8 wce, mask;
 	int i;
 
 	/*
@@ -3632,8 +3632,11 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	ata_msense_caching(dev->id, mpage, false);
 	for (i = 0; i < CACHE_MPAGE_LEN - 2; i++) {
 		if (i == 0)
-			continue;
-		if (mpage[i + 2] != buf[i]) {
+			mask = 0xfb;
+		else
+			mask = 0xff;
+
+		if ((mpage[i + 2] & mask) != (buf[i] & mask)) {
 			*fp = i;
 			return -EINVAL;
 		}
@@ -3666,7 +3669,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 {
 	struct ata_device *dev = qc->dev;
 	char mpage[CONTROL_MPAGE_LEN];
-	u8 d_sense;
+	u8 d_sense, mask;
 	int i;
 
 	/*
@@ -3687,8 +3690,11 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 	ata_msense_control(dev, mpage, false);
 	for (i = 0; i < CONTROL_MPAGE_LEN - 2; i++) {
 		if (i == 0)
-			continue;
-		if (mpage[2 + i] != buf[i]) {
+			mask = 0xfb;
+		else
+			mask = 0xff;
+
+		if ((mpage[2 + i] & mask) != (buf[i] & mask)) {
 			*fp = i;
 			return -EINVAL;
 		}
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1448260 — [PATCH resend v2 2/5] libata-scsi: fix read-only bits checking in ata_mselect_*()

Fromtom.ty89@gmail.com
Date2016-07-22 01:40 +0200
Subject[PATCH resend v2 2/5] libata-scsi: fix read-only bits checking in ata_mselect_*()
Message-ID<rXvJn-60q-19@gated-at.bofh.it>
In reply to#1448249
From: Tom Yan <tom.ty89@gmail.com>

Commit 7780081c1f04 ("libata-scsi: Set information sense field for
invalid parameter") changed how ata_mselect_*() make sure read-only
bits are not modified. The new implementation introduced a bug that
the read-only bits in the byte that has a changeable bit will not
be checked.

Made it check every byte but mask the changeable bit.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index eb5e8ff..4a4e6f1 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3611,7 +3611,7 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	struct ata_taskfile *tf = &qc->tf;
 	struct ata_device *dev = qc->dev;
 	char mpage[CACHE_MPAGE_LEN];
-	u8 wce;
+	u8 wce, mask;
 	int i;
 
 	/*
@@ -3632,8 +3632,11 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	ata_msense_caching(dev->id, mpage, false);
 	for (i = 0; i < CACHE_MPAGE_LEN - 2; i++) {
 		if (i == 0)
-			continue;
-		if (mpage[i + 2] != buf[i]) {
+			mask = 0xfb;
+		else
+			mask = 0xff;
+
+		if ((mpage[i + 2] & mask) != (buf[i] & mask)) {
 			*fp = i;
 			return -EINVAL;
 		}
@@ -3666,7 +3669,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 {
 	struct ata_device *dev = qc->dev;
 	char mpage[CONTROL_MPAGE_LEN];
-	u8 d_sense;
+	u8 d_sense, mask;
 	int i;
 
 	/*
@@ -3687,8 +3690,11 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
 	ata_msense_control(dev, mpage, false);
 	for (i = 0; i < CONTROL_MPAGE_LEN - 2; i++) {
 		if (i == 0)
-			continue;
-		if (mpage[2 + i] != buf[i]) {
+			mask = 0xfb;
+		else
+			mask = 0xff;
+
+		if ((mpage[2 + i] & mask) != (buf[i] & mask)) {
 			*fp = i;
 			return -EINVAL;
 		}
-- 
2.9.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web