[PATCH] ide-tape: Don't leak kernel stack information

[PATCH] ide-tape: Don't leak kernel stack information

[Michael Buesch]

Don't leak kernel stack information through uninitialized structure members.

Signed-off-by: Michael Buesch <mb@bu3sch.de>
Cc: stable@kernel.org

---

This patch is only compile tested.

---
 drivers/ide/ide-tape.c |    1 +
 1 file changed, 1 insertion(+)

--- linux-2.6.orig/drivers/ide/ide-tape.c
+++ linux-2.6/drivers/ide/ide-tape.c
@@ -1057,20 +1057,21 @@ static int idetape_blkdev_ioctl(ide_driv
 
 	debug_log(DBG_PROCS, "Enter %s\n", __func__);
 
 	switch (cmd) {
 	case 0x0340:
 		if (copy_from_user(&config, argp, sizeof(config)))
 			return -EFAULT;
 		tape->best_dsc_rw_freq = config.dsc_rw_frequency;
 		break;
 	case 0x0350:
+		memset(&config, 0, sizeof(config));
 		config.dsc_rw_frequency = (int) tape->best_dsc_rw_freq;
 		config.nr_stages = 1;
 		if (copy_to_user(argp, &config, sizeof(config)))
 			return -EFAULT;
 		break;
 	default:
 		return -EIO;
 	}
 	return 0;
 }

-- 
Greetings, Michael.

Re: [PATCH] ide-tape: Don't leak kernel stack information

[Borislav Petkov]

On Sun, Jul 19, 2009 at 09:15:19PM +0200, Michael Buesch wrote:
> Don't leak kernel stack information through uninitialized structure members.
> 
> Signed-off-by: Michael Buesch <mb@bu3sch.de>
> Cc: stable@kernel.org
> 
> ---
> 
> This patch is only compile tested.
> 
> ---
>  drivers/ide/ide-tape.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> --- linux-2.6.orig/drivers/ide/ide-tape.c
> +++ linux-2.6/drivers/ide/ide-tape.c
> @@ -1057,20 +1057,21 @@ static int idetape_blkdev_ioctl(ide_driv
>  
>  	debug_log(DBG_PROCS, "Enter %s\n", __func__);
>  
>  	switch (cmd) {
>  	case 0x0340:
>  		if (copy_from_user(&config, argp, sizeof(config)))
>  			return -EFAULT;
>  		tape->best_dsc_rw_freq = config.dsc_rw_frequency;
>  		break;
>  	case 0x0350:
> +		memset(&config, 0, sizeof(config));

Well, I can't find config.dsc_media_access_frequency as being used
anywhere since the git years of the kernel. I found¹ some archaic
kernels from 1995 (1.3 series) which used to have IDETAPE_RESET_IOCTL
defined as 0x0350 but can't seem to find any userspace use of that
ioctl.

If there's none, you might just as well remove
config.dsc_media_access_frequency as an alternative solution.

@Bart: Any historic info I'm missing here?


¹http://www.google.com/search?q=IDETAPE_RESET_IOCTL

-- 
Regards/Gruss,
    Boris.

Re: [PATCH] ide-tape: Don't leak kernel stack information

[Borislav Petkov]

On Tue, Jul 21, 2009 at 12:06 PM, Bartlomiej
Zolnierkiewicz<bzolnier@gmail.com> wrote:
> On Monday 20 July 2009 09:38:14 Borislav Petkov wrote:
>> On Sun, Jul 19, 2009 at 09:15:19PM +0200, Michael Buesch wrote:
>> > Don't leak kernel stack information through uninitialized structure members.
>> >
>> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
>> > Cc: stable@kernel.org
>> >
>> > ---
>> >
>> > This patch is only compile tested.
>> >
>> > ---
>> >  drivers/ide/ide-tape.c |    1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > --- linux-2.6.orig/drivers/ide/ide-tape.c
>> > +++ linux-2.6/drivers/ide/ide-tape.c
>> > @@ -1057,20 +1057,21 @@ static int idetape_blkdev_ioctl(ide_driv
>> >
>> >     debug_log(DBG_PROCS, "Enter %s\n", __func__);
>> >
>> >     switch (cmd) {
>> >     case 0x0340:
>> >             if (copy_from_user(&config, argp, sizeof(config)))
>> >                     return -EFAULT;
>> >             tape->best_dsc_rw_freq = config.dsc_rw_frequency;
>> >             break;
>> >     case 0x0350:
>> > +           memset(&config, 0, sizeof(config));
>>
>> Well, I can't find config.dsc_media_access_frequency as being used
>> anywhere since the git years of the kernel. I found¹ some archaic
>> kernels from 1995 (1.3 series) which used to have IDETAPE_RESET_IOCTL
>> defined as 0x0350 but can't seem to find any userspace use of that
>> ioctl.
>>
>> If there's none, you might just as well remove
>> config.dsc_media_access_frequency as an alternative solution.
>>
>> @Bart: Any historic info I'm missing here?
>
> We need to preserve struct idetape_config layout to not break the ioctl
> (regardless if the field is really used by some user-space apps or not)..

I know that. However, I can't seem to find any definition of that
ioctl, ioctl_list(2) manpage doesn't document it, so my question was
whether there is an agreed-upon definition of that ioctl and of struct
config at all. If not, we could simply remove that variable since the
struct layout is simply arbitrary in that case. Especially if there's
no corresponing driver functionality on the kernel side which makes
config.dsc_media_access_frequency the more useless.

-- 
Regards/Gruss,
Boris

Re: [PATCH] ide-tape: Don't leak kernel stack information

[Michael Buesch]

On Monday 20 July 2009 09:38:14 Borislav Petkov wrote:
> On Sun, Jul 19, 2009 at 09:15:19PM +0200, Michael Buesch wrote:
> > Don't leak kernel stack information through uninitialized structure members.
> > 
> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> > Cc: stable@kernel.org
> > 
> > ---
> > 
> > This patch is only compile tested.
> > 
> > ---
> >  drivers/ide/ide-tape.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > --- linux-2.6.orig/drivers/ide/ide-tape.c
> > +++ linux-2.6/drivers/ide/ide-tape.c
> > @@ -1057,20 +1057,21 @@ static int idetape_blkdev_ioctl(ide_driv
> >  
> >  	debug_log(DBG_PROCS, "Enter %s\n", __func__);
> >  
> >  	switch (cmd) {
> >  	case 0x0340:
> >  		if (copy_from_user(&config, argp, sizeof(config)))
> >  			return -EFAULT;
> >  		tape->best_dsc_rw_freq = config.dsc_rw_frequency;
> >  		break;
> >  	case 0x0350:
> > +		memset(&config, 0, sizeof(config));
> 
> Well, I can't find config.dsc_media_access_frequency as being used
> anywhere since the git years of the kernel. I found¹ some archaic
> kernels from 1995 (1.3 series) which used to have IDETAPE_RESET_IOCTL
> defined as 0x0350 but can't seem to find any userspace use of that
> ioctl.
> 
> If there's none, you might just as well remove
> config.dsc_media_access_frequency as an alternative solution.
> 
> @Bart: Any historic info I'm missing here?
> 
> 
> ¹http://www.google.com/search?q=IDETAPE_RESET_IOCTL
> 

Well, I don't feel so good changing the ABI of ancient drivers. So
I think it's best to just fix the bug (zero out the struct) instead of removing
the whole call. Who knows. Maybe some proprietary program in the depths of some
corporation's servers uses this ioctl...

So let's just zero out the structure properly to avoid the possibility of leaking
kernel stack information.

-- 
Greetings, Michael.

Re: [PATCH] ide-tape: Don't leak kernel stack information

[Borislav Petkov]

On Tue, Jul 21, 2009 at 2:10 PM, Bartlomiej
Zolnierkiewicz<bzolnier@gmail.com> wrote:

[..]

> Either it stays the way it is or we may eventually remove it completely
> (if really nothing uses it, though I still don't see much point in it
> given "the deep maintenance" mode of IDE subsystem).

Ok, let's opt for the less intrusive/no breakage option. The deep
maintenance mode argument clearly prevails here.

Acked-by: Borislav Petkov <petkovbb@gmail.com>.

-- 
Regards/Gruss,
Boris

Re: [PATCH] ide-tape: Don't leak kernel stack information

[Bartlomiej Zolnierkiewicz]

On Monday 20 July 2009 09:38:14 Borislav Petkov wrote:
> On Sun, Jul 19, 2009 at 09:15:19PM +0200, Michael Buesch wrote:
> > Don't leak kernel stack information through uninitialized structure members.
> > 
> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> > Cc: stable@kernel.org
> > 
> > ---
> > 
> > This patch is only compile tested.
> > 
> > ---
> >  drivers/ide/ide-tape.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > --- linux-2.6.orig/drivers/ide/ide-tape.c
> > +++ linux-2.6/drivers/ide/ide-tape.c
> > @@ -1057,20 +1057,21 @@ static int idetape_blkdev_ioctl(ide_driv
> >  
> >  	debug_log(DBG_PROCS, "Enter %s\n", __func__);
> >  
> >  	switch (cmd) {
> >  	case 0x0340:
> >  		if (copy_from_user(&config, argp, sizeof(config)))
> >  			return -EFAULT;
> >  		tape->best_dsc_rw_freq = config.dsc_rw_frequency;
> >  		break;
> >  	case 0x0350:
> > +		memset(&config, 0, sizeof(config));
> 
> Well, I can't find config.dsc_media_access_frequency as being used
> anywhere since the git years of the kernel. I found¹ some archaic
> kernels from 1995 (1.3 series) which used to have IDETAPE_RESET_IOCTL
> defined as 0x0350 but can't seem to find any userspace use of that
> ioctl.
> 
> If there's none, you might just as well remove
> config.dsc_media_access_frequency as an alternative solution.
> 
> @Bart: Any historic info I'm missing here?

We need to preserve struct idetape_config layout to not break the ioctl
(regardless if the field is really used by some user-space apps or not)..

Re: [PATCH] ide-tape: Don't leak kernel stack information

[Bartlomiej Zolnierkiewicz]

On Monday 20 July 2009 12:45:37 Borislav Petkov wrote:
> On Tue, Jul 21, 2009 at 12:06 PM, Bartlomiej
> Zolnierkiewicz<bzolnier@gmail.com> wrote:
> > On Monday 20 July 2009 09:38:14 Borislav Petkov wrote:
> >> On Sun, Jul 19, 2009 at 09:15:19PM +0200, Michael Buesch wrote:
> >> > Don't leak kernel stack information through uninitialized structure members.
> >> >
> >> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> >> > Cc: stable@kernel.org
> >> >
> >> > ---
> >> >
> >> > This patch is only compile tested.
> >> >
> >> > ---
> >> >  drivers/ide/ide-tape.c |    1 +
> >> >  1 file changed, 1 insertion(+)
> >> >
> >> > --- linux-2.6.orig/drivers/ide/ide-tape.c
> >> > +++ linux-2.6/drivers/ide/ide-tape.c
> >> > @@ -1057,20 +1057,21 @@ static int idetape_blkdev_ioctl(ide_driv
> >> >
> >> >     debug_log(DBG_PROCS, "Enter %s\n", __func__);
> >> >
> >> >     switch (cmd) {
> >> >     case 0x0340:
> >> >             if (copy_from_user(&config, argp, sizeof(config)))
> >> >                     return -EFAULT;
> >> >             tape->best_dsc_rw_freq = config.dsc_rw_frequency;
> >> >             break;
> >> >     case 0x0350:
> >> > +           memset(&config, 0, sizeof(config));
> >>
> >> Well, I can't find config.dsc_media_access_frequency as being used
> >> anywhere since the git years of the kernel. I found¹ some archaic
> >> kernels from 1995 (1.3 series) which used to have IDETAPE_RESET_IOCTL
> >> defined as 0x0350 but can't seem to find any userspace use of that
> >> ioctl.
> >>
> >> If there's none, you might just as well remove
> >> config.dsc_media_access_frequency as an alternative solution.
> >>
> >> @Bart: Any historic info I'm missing here?
> >
> > We need to preserve struct idetape_config layout to not break the ioctl
> > (regardless if the field is really used by some user-space apps or not)..
> 
> I know that. However, I can't seem to find any definition of that
> ioctl, ioctl_list(2) manpage doesn't document it, so my question was
> whether there is an agreed-upon definition of that ioctl and of struct
> config at all. If not, we could simply remove that variable since the
> struct layout is simply arbitrary in that case. Especially if there's

It is not, user-space (theoretically) depends on the current layout
(we copy_to_user() the _whole_ structure).

> no corresponing driver functionality on the kernel side which makes
> config.dsc_media_access_frequency the more useless.

We don't do arbitrary changes to existing ioctls.

Either it stays the way it is or we may eventually remove it completely
(if really nothing uses it, though I still don't see much point in it
given "the deep maintenance" mode of IDE subsystem).

Re: [PATCH] ide-tape: Don't leak kernel stack information

[David Miller]

From: Borislav Petkov <petkovbb@googlemail.com>
Date: Mon, 20 Jul 2009 14:49:12 +0200

> On Tue, Jul 21, 2009 at 2:10 PM, Bartlomiej
> Zolnierkiewicz<bzolnier@gmail.com> wrote:
> 
> [..]
> 
>> Either it stays the way it is or we may eventually remove it completely
>> (if really nothing uses it, though I still don't see much point in it
>> given "the deep maintenance" mode of IDE subsystem).
> 
> Ok, let's opt for the less intrusive/no breakage option. The deep
> maintenance mode argument clearly prevails here.
> 
> Acked-by: Borislav Petkov <petkovbb@gmail.com>.

Agreed, applied and queued to -stable, thanks.