[PATCH] st.c: possible memory use after free after MTSETBLK ioctl

[PATCH] st.c: possible memory use after free after MTSETBLK ioctl

[David Jeffery]

A memory use after free bug can manifest if the MTSETBLK or SET_DENS_AND_BLK
ioctl features are used to set the tape's blocksize from 0 to non-zero.
After the driver sets the new block size, in this one case it calls
normalize_buffer() to free the device's internal data buffers.  However, the
ioctl code assumes there is always a buffer and does not check or allocate
a buffer if there isn't one.  So any following ioctl calls can corrupt
a part of memory by writing data to memory that the st driver had freed.

This small patch forces the st driver to allocate a minimal one page buffer
using enlarge_buffer() after the previous buffers were freed.


Signed-of-by: David Jeffery <djeffery@redhat.com>


--- a/drivers/scsi/st.c	2009-09-24 12:34:14.000000000 -0400
+++ b/drivers/scsi/st.c	2009-09-23 22:10:17.000000000 -0400
@@ -2776,8 +2776,12 @@
 			int old_block_size = STp->block_size;
 			STp->block_size = arg & MT_ST_BLKSIZE_MASK;
 			if (STp->block_size != 0) {
-				if (old_block_size == 0)
+				if (old_block_size == 0){
 					normalize_buffer(STp->buffer);
+					if (!enlarge_buffer(STp->buffer, PAGE_SIZE, STp->restr_dma)) {
+						printk(KERN_ERR "No st buffer!\n");
+					}
+				}
 				(STp->buffer)->buffer_blocks =
 				    (STp->buffer)->buffer_size / STp->block_size;
 			}

Re: [PATCH] st.c: possible memory use after free after MTSETBLK ioctl

[Kai Makisara]

On Thu, 24 Sep 2009, David Jeffery wrote:

> A memory use after free bug can manifest if the MTSETBLK or SET_DENS_AND_BLK
> ioctl features are used to set the tape's blocksize from 0 to non-zero.
> After the driver sets the new block size, in this one case it calls
> normalize_buffer() to free the device's internal data buffers.  However, the
> ioctl code assumes there is always a buffer and does not check or allocate
> a buffer if there isn't one.  So any following ioctl calls can corrupt
> a part of memory by writing data to memory that the st driver had freed.
> 
> This small patch forces the st driver to allocate a minimal one page buffer
> using enlarge_buffer() after the previous buffers were freed.
> 
> 
> Signed-of-by: David Jeffery <djeffery@redhat.com>
> 
> 
> --- a/drivers/scsi/st.c	2009-09-24 12:34:14.000000000 -0400
> +++ b/drivers/scsi/st.c	2009-09-23 22:10:17.000000000 -0400
> @@ -2776,8 +2776,12 @@
>  			int old_block_size = STp->block_size;
>  			STp->block_size = arg & MT_ST_BLKSIZE_MASK;
>  			if (STp->block_size != 0) {
> -				if (old_block_size == 0)
> +				if (old_block_size == 0){
>  					normalize_buffer(STp->buffer);
> +					if (!enlarge_buffer(STp->buffer, PAGE_SIZE, STp->restr_dma)) {
> +						printk(KERN_ERR "No st buffer!\n");
> +					}
> +				}
>  				(STp->buffer)->buffer_blocks =
>  				    (STp->buffer)->buffer_size / STp->block_size;
>  			}

Good catch. This bug probably has not manifested itself because the next 
commands have either been ioctls not needing a buffer or a read/write that 
has enlarged the buffer.

The patch has one problem: it prints an error if the page can't be 
allocated but it does not do prevent using a non-allocated buffer in this 
case. Although this is a nearly theoretical possibility, it should be 
handled in some way. At least a comment should be added.

Is the patch against a current kernel? The line numbers don't match either 
2.6.30 or 2.6.31?

Thanks,
Kai

Re: [PATCH] st.c: possible memory use after free after MTSETBLK ioctl

[Kai Makisara]

On Fri, 25 Sep 2009, Kai Makisara wrote:

> On Thu, 24 Sep 2009, David Jeffery wrote:
> 
> > A memory use after free bug can manifest if the MTSETBLK or SET_DENS_AND_BLK
> > ioctl features are used to set the tape's blocksize from 0 to non-zero.
> > After the driver sets the new block size, in this one case it calls
> > normalize_buffer() to free the device's internal data buffers.  However, the
> > ioctl code assumes there is always a buffer and does not check or allocate
> > a buffer if there isn't one.  So any following ioctl calls can corrupt
> > a part of memory by writing data to memory that the st driver had freed.
> > 
> > This small patch forces the st driver to allocate a minimal one page buffer
> > using enlarge_buffer() after the previous buffers were freed.
> > 
> > 
> > Signed-of-by: David Jeffery <djeffery@redhat.com>
> > 
> > 
> > --- a/drivers/scsi/st.c	2009-09-24 12:34:14.000000000 -0400
> > +++ b/drivers/scsi/st.c	2009-09-23 22:10:17.000000000 -0400
> > @@ -2776,8 +2776,12 @@
> >  			int old_block_size = STp->block_size;
> >  			STp->block_size = arg & MT_ST_BLKSIZE_MASK;
> >  			if (STp->block_size != 0) {
> > -				if (old_block_size == 0)
> > +				if (old_block_size == 0){
> >  					normalize_buffer(STp->buffer);
> > +					if (!enlarge_buffer(STp->buffer, PAGE_SIZE, STp->restr_dma)) {
> > +						printk(KERN_ERR "No st buffer!\n");
> > +					}
> > +				}
> >  				(STp->buffer)->buffer_blocks =
> >  				    (STp->buffer)->buffer_size / STp->block_size;
> >  			}
> 
> Good catch. This bug probably has not manifested itself because the next 
> commands have either been ioctls not needing a buffer or a read/write that 
> has enlarged the buffer.
> 
> The patch has one problem: it prints an error if the page can't be 
> allocated but it does not do prevent using a non-allocated buffer in this 
> case. Although this is a nearly theoretical possibility, it should be 
> handled in some way. At least a comment should be added.
> 
Having thought this a little more, it would probably be best to remove the 
call to normalize_buffer() instead of trying to cope with it failing. This 
just means that some memory may stay allocated longer. In the usage 
scenarios coming into my mind, the buffer is either small or it will be 
reallocated.

Would you like to make the patch?

Thanks,
Kai

Re: [PATCH] st.c: possible memory use after free after MTSETBLK ioctl

[Kai Makisara]

On Mon, 28 Sep 2009, David Jeffery wrote:

> > On Fri, 25 Sep 2009, Kai Makisara wrote:
> > 
> > Having thought this a little more, it would probably be best to remove
> > the 
> > call to normalize_buffer() instead of trying to cope with it failing.
> > This 
> > just means that some memory may stay allocated longer. In the usage 
> > scenarios coming into my mind, the buffer is either small or it will
> > be 
> > reallocated.
> > 
> > Would you like to make the patch?
> > 
> > Thanks,
> > Kai
> 
> How does this look?  Compiles and passes initial tests.  I'd left the
> normalize_buffer() in originally thinking there must be a reason for
> it, but you're right it just seems to make buffer handling problematic.
> This patch removes the normalize_buffer() call and the specialness of
> changing from a 0 to non-zero blocksize to fix the possible use of
> memory after it has been freed by the st driver.
> 
> signed-off-by: David Jeffery <djeffery@redhat.com>
> 
> --- a/drivers/scsi/st.c	2009-09-28 13:29:31.000000000 -0400
> +++ b/drivers/scsi/st.c	2009-09-28 13:35:32.000000000 -0400
> @@ -2859,11 +2859,8 @@
>  			ioctl_result = st_int_ioctl(STp, MTBSF, 1);
>  
>  		if (cmd_in == MTSETBLK || cmd_in == SET_DENS_AND_BLK) {
> -			int old_block_size = STp->block_size;
>  			STp->block_size = arg & MT_ST_BLKSIZE_MASK;
>  			if (STp->block_size != 0) {
> -				if (old_block_size == 0)
> -					normalize_buffer(STp->buffer);
>  				(STp->buffer)->buffer_blocks =
>  				    (STp->buffer)->buffer_size / STp->block_size;
>  			}
> 
Looks good to me. There were reasons when it was inserted, but many things 
have changed since then: memory was expensive, I/O always used the buffer, 
normalize_buffer() did not free the last segment, etc.

Acked-by: Kai Makisara <kai.makisara@kolumbus.fi>

Thanks,
Kai

Re: [PATCH] st.c: possible memory use after free after MTSETBLK ioctl

[David Jeffery]

> On Fri, 25 Sep 2009, Kai Makisara wrote:
> 
> Having thought this a little more, it would probably be best to remove
> the 
> call to normalize_buffer() instead of trying to cope with it failing.
> This 
> just means that some memory may stay allocated longer. In the usage 
> scenarios coming into my mind, the buffer is either small or it will
> be 
> reallocated.
> 
> Would you like to make the patch?
> 
> Thanks,
> Kai

How does this look?  Compiles and passes initial tests.  I'd left the
normalize_buffer() in originally thinking there must be a reason for
it, but you're right it just seems to make buffer handling problematic.
This patch removes the normalize_buffer() call and the specialness of
changing from a 0 to non-zero blocksize to fix the possible use of
memory after it has been freed by the st driver.

signed-off-by: David Jeffery <djeffery@redhat.com>

--- a/drivers/scsi/st.c	2009-09-28 13:29:31.000000000 -0400
+++ b/drivers/scsi/st.c	2009-09-28 13:35:32.000000000 -0400
@@ -2859,11 +2859,8 @@
 			ioctl_result = st_int_ioctl(STp, MTBSF, 1);
 
 		if (cmd_in == MTSETBLK || cmd_in == SET_DENS_AND_BLK) {
-			int old_block_size = STp->block_size;
 			STp->block_size = arg & MT_ST_BLKSIZE_MASK;
 			if (STp->block_size != 0) {
-				if (old_block_size == 0)
-					normalize_buffer(STp->buffer);
 				(STp->buffer)->buffer_blocks =
 				    (STp->buffer)->buffer_size / STp->block_size;
 			}