[resend,3/5] libata-scsi: fix overflow in mode page copy

Commit Message

Tom Yan July 21, 2016, 6:41 p.m. UTC

From: Tom Yan <[e-mail]>

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 <[e-mail]>

Comments

Tejun Heo July 21, 2016, 9:17 p.m. UTC | #1

Hello,

On Fri, Jul 22, 2016 at 02:41:52AM +0800, [e-mail] wrote:
> From: Tom Yan <[e-mail]>
> 
> 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.

Tom Yan July 21, 2016, 9:39 p.m. UTC | #2

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 <[e-mail]> wrote:
> Hello,
>
> On Fri, Jul 22, 2016 at 02:41:52AM +0800, [e-mail] wrote:
>> From: Tom Yan <[e-mail]>
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tejun Heo July 21, 2016, 9:42 p.m. UTC | #3

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.

Patch

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;