[PATCH] Fix sg_io_hdr.info corruption.

[PATCH] Fix sg_io_hdr.info corruption.

[Alexey Zaytsev]

sizeof(unsigned (short)) is actually sizeof(function), == 1.
Spotted by sparse.

Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
---
 fs/compat_ioctl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 5235c67..7c2d617 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -784,7 +784,7 @@ static int sg_ioctl_trans(unsigned int fd, unsigned int cmd, unsigned long arg)
 
 	if (copy_in_user(&sgio->status, &sgio32->status,
 			 (4 * sizeof(unsigned char)) +
-			 (2 * sizeof(unsigned (short))) +
+			 (2 * sizeof(unsigned short)) +
 			 (3 * sizeof(int))))
 		return -EFAULT;
 

Re: [PATCH] Fix sg_io_hdr.info corruption.

[Andrew Morton]

On Sun, 28 Dec 2008 17:50:35 +0300
Alexey Zaytsev <alexey.zaytsev@gmail.com> wrote:

> sizeof(unsigned (short)) is actually sizeof(function), == 1.
> Spotted by sparse.
> 
> Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
> ---
>  fs/compat_ioctl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> index 5235c67..7c2d617 100644
> --- a/fs/compat_ioctl.c
> +++ b/fs/compat_ioctl.c
> @@ -784,7 +784,7 @@ static int sg_ioctl_trans(unsigned int fd, unsigned int cmd, unsigned long arg)
>  
>  	if (copy_in_user(&sgio->status, &sgio32->status,
>  			 (4 * sizeof(unsigned char)) +
> -			 (2 * sizeof(unsigned (short))) +
> +			 (2 * sizeof(unsigned short)) +
>  			 (3 * sizeof(int))))
>  		return -EFAULT;

gack.

akpm:/home/akpm> cat t.c
main()
{
	        printf("%d\n", sizeof(unsigned (short)));
	        printf("%d\n", sizeof(unsigned short));
}
akpm:/home/akpm> ./a.out
1
2


the code has been like this for years and years.  Why hasn't anyone
noticed?  

Re: [PATCH] Fix sg_io_hdr.info corruption.

[FUJITA Tomonori]

On Tue, 30 Dec 2008 15:46:03 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Sun, 28 Dec 2008 17:50:35 +0300
> Alexey Zaytsev <alexey.zaytsev@gmail.com> wrote:
> 
> > sizeof(unsigned (short)) is actually sizeof(function), == 1.
> > Spotted by sparse.
> > 
> > Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
> > ---
> >  fs/compat_ioctl.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> > index 5235c67..7c2d617 100644
> > --- a/fs/compat_ioctl.c
> > +++ b/fs/compat_ioctl.c
> > @@ -784,7 +784,7 @@ static int sg_ioctl_trans(unsigned int fd, unsigned int cmd, unsigned long arg)
> >  
> >  	if (copy_in_user(&sgio->status, &sgio32->status,
> >  			 (4 * sizeof(unsigned char)) +
> > -			 (2 * sizeof(unsigned (short))) +
> > +			 (2 * sizeof(unsigned short)) +
> >  			 (3 * sizeof(int))))
> >  		return -EFAULT;
> 
> gack.
> 
> akpm:/home/akpm> cat t.c
> main()
> {
> 	        printf("%d\n", sizeof(unsigned (short)));
> 	        printf("%d\n", sizeof(unsigned short));
> }
> akpm:/home/akpm> ./a.out
> 1
> 2
> 
> 
> the code has been like this for years and years.  Why hasn't anyone
> noticed?  

The members from 'status' in struct sg_io_hdr to the last are used to
transfer information from kernel to user space. The values that user
space sets are just ignored.

typedef struct sg_io_hdr
{
    int interface_id;           /* [i] 'S' for SCSI generic (required) */
    int dxfer_direction;        /* [i] data transfer direction  */
    unsigned char cmd_len;      /* [i] SCSI command length ( <= 16 bytes) */
    unsigned char mx_sb_len;    /* [i] max length to write to sbp */
    unsigned short iovec_count; /* [i] 0 implies no scatter gather */
    unsigned int dxfer_len;     /* [i] byte count of data transfer */
    void __user *dxferp;	/* [i], [*io] points to data transfer memory
					      or scatter gather list */
    unsigned char __user *cmdp; /* [i], [*i] points to command to perform */
    void __user *sbp;		/* [i], [*o] points to sense_buffer memory */
    unsigned int timeout;       /* [i] MAX_UINT->no timeout (unit: millisec) */
    unsigned int flags;         /* [i] 0 -> default, see SG_FLAG... */
    int pack_id;                /* [i->o] unused internally (normally) */
    void __user * usr_ptr;      /* [i->o] unused internally */
    unsigned char status;       /* [o] scsi status */
    unsigned char masked_status;/* [o] shifted, masked scsi status */
    unsigned char msg_status;   /* [o] messaging level data (optional) */
    unsigned char sb_len_wr;    /* [o] byte count actually written to sbp */
    unsigned short host_status; /* [o] errors from host adapter */
    unsigned short driver_status;/* [o] errors from software driver */
    int resid;                  /* [o] dxfer_len - actual_transferred */
    unsigned int duration;      /* [o] time taken by cmd (unit: millisec) */
    unsigned int info;          /* [o] auxiliary information */
} sg_io_hdr_t;  /* 64 bytes long (on i386) */

Re: [PATCH] Fix sg_io_hdr.info corruption.

[Andrew Morton]

On Wed, 31 Dec 2008 18:08:02 +0900 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> On Tue, 30 Dec 2008 15:46:03 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Sun, 28 Dec 2008 17:50:35 +0300
> > Alexey Zaytsev <alexey.zaytsev@gmail.com> wrote:
> > 
> > > sizeof(unsigned (short)) is actually sizeof(function), == 1.
> > > Spotted by sparse.
> > > 
> > > Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
> > > ---
> > >  fs/compat_ioctl.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> > > index 5235c67..7c2d617 100644
> > > --- a/fs/compat_ioctl.c
> > > +++ b/fs/compat_ioctl.c
> > > @@ -784,7 +784,7 @@ static int sg_ioctl_trans(unsigned int fd, unsigned int cmd, unsigned long arg)
> > >  
> > >  	if (copy_in_user(&sgio->status, &sgio32->status,
> > >  			 (4 * sizeof(unsigned char)) +
> > > -			 (2 * sizeof(unsigned (short))) +
> > > +			 (2 * sizeof(unsigned short)) +
> > >  			 (3 * sizeof(int))))
> > >  		return -EFAULT;
> > 
> > gack.
> > 
> > akpm:/home/akpm> cat t.c
> > main()
> > {
> > 	        printf("%d\n", sizeof(unsigned (short)));
> > 	        printf("%d\n", sizeof(unsigned short));
> > }
> > akpm:/home/akpm> ./a.out
> > 1
> > 2
> > 
> > 
> > the code has been like this for years and years.  Why hasn't anyone
> > noticed?  
> 
> The members from 'status' in struct sg_io_hdr to the last are used to
> transfer information from kernel to user space. The values that user
> space sets are just ignored.

OK.

> typedef struct sg_io_hdr
> {
>     int interface_id;           /* [i] 'S' for SCSI generic (required) */
>     int dxfer_direction;        /* [i] data transfer direction  */
>     unsigned char cmd_len;      /* [i] SCSI command length ( <= 16 bytes) */
>     unsigned char mx_sb_len;    /* [i] max length to write to sbp */
>     unsigned short iovec_count; /* [i] 0 implies no scatter gather */
>     unsigned int dxfer_len;     /* [i] byte count of data transfer */
>     void __user *dxferp;	/* [i], [*io] points to data transfer memory
> 					      or scatter gather list */
>     unsigned char __user *cmdp; /* [i], [*i] points to command to perform */
>     void __user *sbp;		/* [i], [*o] points to sense_buffer memory */
>     unsigned int timeout;       /* [i] MAX_UINT->no timeout (unit: millisec) */
>     unsigned int flags;         /* [i] 0 -> default, see SG_FLAG... */
>     int pack_id;                /* [i->o] unused internally (normally) */
>     void __user * usr_ptr;      /* [i->o] unused internally */
>     unsigned char status;       /* [o] scsi status */
>     unsigned char masked_status;/* [o] shifted, masked scsi status */
>     unsigned char msg_status;   /* [o] messaging level data (optional) */
>     unsigned char sb_len_wr;    /* [o] byte count actually written to sbp */
>     unsigned short host_status; /* [o] errors from host adapter */
>     unsigned short driver_status;/* [o] errors from software driver */
>     int resid;                  /* [o] dxfer_len - actual_transferred */
>     unsigned int duration;      /* [o] time taken by cmd (unit: millisec) */
>     unsigned int info;          /* [o] auxiliary information */
> } sg_io_hdr_t;  /* 64 bytes long (on i386) */

So if for some reason the kernel doesn't write to .info, we'll leak two
bytes of kernel memory. 

(no subject)

[Alexey Zaytsev]

On Wed, Dec 31, 2008 at 12:08, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> On Tue, 30 Dec 2008 15:46:03 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
[...]
>> the code has been like this for years and years.  Why hasn't anyone
>> noticed?
>
> The members from 'status' in struct sg_io_hdr to the last are used to
> transfer information from kernel to user space. The values that user
> space sets are just ignored.
>

Then probably there is no need to copy those fields, right?
There should be no data leak from the kernel, as sgio is
allocated on the userspace stack, and the appropriate ioctl
handler should set/zero all those fields anyway, as it expects
them to come directly from the user (did not check).
So, at worst the user will get it's own garbage insted of the
values he left in the fields that the kernel was supposed to
set.

If so, please could drop my previous patch and take this one.

From: Alexey Zaytsev <alexey.zaytsev@gmail.com>
Subject: [PATCH] Don't perform unneeded copy.

FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> says:

The members from 'status' in struct sg_io_hdr to the last are used to
transfer information from kernel to user space. The values that user
space sets are just ignored.

Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
---
 fs/compat_ioctl.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 5235c67..23b1f5a 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -782,12 +782,6 @@ static int sg_ioctl_trans(unsigned int fd, unsigned int cmd, unsigned long arg)
 	if (put_user(compat_ptr(data), &sgio->usr_ptr))
 		return -EFAULT;
 
-	if (copy_in_user(&sgio->status, &sgio32->status,
-			 (4 * sizeof(unsigned char)) +
-			 (2 * sizeof(unsigned (short))) +
-			 (3 * sizeof(int))))
-		return -EFAULT;
-
 	err = sys_ioctl(fd, cmd, (unsigned long) sgio);
 
 	if (err >= 0) {

Re: [PATCH] Fix sg_io_hdr.info corruption.

[Alexey Zaytsev]

[resending in case you missed the one I sent with broken headers]

On Wed, Dec 31, 2008 at 12:08, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> On Tue, 30 Dec 2008 15:46:03 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
[...]
>> the code has been like this for years and years.  Why hasn't anyone
>> noticed?
>
> The members from 'status' in struct sg_io_hdr to the last are used to
> transfer information from kernel to user space. The values that user
> space sets are just ignored.
>

Then probably there is no need to copy those fields, right?
There should be no data leak from the kernel, as sgio is
allocated on the userspace stack, and the appropriate ioctl
handler should set/zero all those fields anyway, as it expects
them to come directly from the user (did not check).
So, in the worst case the user gets his own garbage insted of
the values he left in the fields that the kernel was supposed
to set.

If so, please drop my previous patch and take this one.

From: Alexey Zaytsev <alexey.zaytsev@gmail.com>
Subject: [PATCH] Don't perform unneeded copy.

FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> says:

The members from 'status' in struct sg_io_hdr to the last are used to
transfer information from kernel to user space. The values that user
space sets are just ignored.

Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
---
 fs/compat_ioctl.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 5235c67..23b1f5a 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -782,12 +782,6 @@ static int sg_ioctl_trans(unsigned int fd, unsigned int cmd, unsigned long arg)
 	if (put_user(compat_ptr(data), &sgio->usr_ptr))
 		return -EFAULT;
 
-	if (copy_in_user(&sgio->status, &sgio32->status,
-			 (4 * sizeof(unsigned char)) +
-			 (2 * sizeof(unsigned (short))) +
-			 (3 * sizeof(int))))
-		return -EFAULT;
-
 	err = sys_ioctl(fd, cmd, (unsigned long) sgio);
 
 	if (err >= 0) {

Re: [PATCH] Fix sg_io_hdr.info corruption.

[Douglas Gilbert]

Alexey Zaytsev wrote:
> [resending in case you missed the one I sent with broken headers]
> 
> On Wed, Dec 31, 2008 at 12:08, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>> On Tue, 30 Dec 2008 15:46:03 -0800
>> Andrew Morton <akpm@linux-foundation.org> wrote:
> [...]
>>> the code has been like this for years and years.  Why hasn't anyone
>>> noticed?
>> The members from 'status' in struct sg_io_hdr to the last are used to
>> transfer information from kernel to user space. The values that user
>> space sets are just ignored.
>>
> 
> Then probably there is no need to copy those fields, right?

Correct.

Doug Gilbert

> There should be no data leak from the kernel, as sgio is
> allocated on the userspace stack, and the appropriate ioctl
> handler should set/zero all those fields anyway, as it expects
> them to come directly from the user (did not check).
> So, in the worst case the user gets his own garbage insted of
> the values he left in the fields that the kernel was supposed
> to set.
> 
> If so, please drop my previous patch and take this one.
> 
> From: Alexey Zaytsev <alexey.zaytsev@gmail.com>
> Subject: [PATCH] Don't perform unneeded copy.
> 
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> says:
> 
> The members from 'status' in struct sg_io_hdr to the last are used to
> transfer information from kernel to user space. The values that user
> space sets are just ignored.
> 
> Signed-off-by: Alexey Zaytsev <alexey.zaytsev@gmail.com>
> ---
>  fs/compat_ioctl.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> index 5235c67..23b1f5a 100644
> --- a/fs/compat_ioctl.c
> +++ b/fs/compat_ioctl.c
> @@ -782,12 +782,6 @@ static int sg_ioctl_trans(unsigned int fd, unsigned int cmd, unsigned long arg)
>  	if (put_user(compat_ptr(data), &sgio->usr_ptr))
>  		return -EFAULT;
>  
> -	if (copy_in_user(&sgio->status, &sgio32->status,
> -			 (4 * sizeof(unsigned char)) +
> -			 (2 * sizeof(unsigned (short))) +
> -			 (3 * sizeof(int))))
> -		return -EFAULT;
> -
>  	err = sys_ioctl(fd, cmd, (unsigned long) sgio);
>  
>  	if (err >= 0) {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>