2010-11-10 14:08:42

by Hillf Danton

Subject: [PATCH] fix vulnerability of the release method of file operations in Block layer SCSI generic driver

The computation context setup by previous opening the bsg file could
not survive following open/release operations upon the same file
object.

The vulnerability is fixed by deferring the cleanup operation until necessary.

Signed-off-by: Hillf Danton <[email protected]>
---

--- a/block/bsg.c 2010-09-13 07:07:38.000000000 +0800
+++ b/block/bsg.c 2010-11-10 21:43:58.000000000 +0800
@@ -858,7 +858,8 @@ static int bsg_release(struct inode *ino
{
struct bsg_device *bd = file->private_data;

- file->private_data = NULL;
+ if (1 == atomic_read(&bd->ref_count))
+ file->private_data = NULL;
return bsg_put_device(bd);
}

2010-11-10 15:18:28

by Matthew Wilcox

Subject: Re: [PATCH] fix vulnerability of the release method of file operations in Block layer SCSI generic driver

On Wed, Nov 10, 2010 at 10:08:37PM +0800, Hillf Danton wrote:
> The computation context setup by previous opening the bsg file could
> not survive following open/release operations upon the same file
> object.

Umm .. release is called on final close of a file, not on every close
of a file.

> The vulnerability is fixed by deferring the cleanup operation until necessary.
>
> Signed-off-by: Hillf Danton <[email protected]>
> ---
>
> --- a/block/bsg.c 2010-09-13 07:07:38.000000000 +0800
> +++ b/block/bsg.c 2010-11-10 21:43:58.000000000 +0800
> @@ -858,7 +858,8 @@ static int bsg_release(struct inode *ino
> {
> struct bsg_device *bd = file->private_data;
>
> - file->private_data = NULL;
> + if (1 == atomic_read(&bd->ref_count))
> + file->private_data = NULL;
> return bsg_put_device(bd);
> }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2010-11-11 13:43:39

by Hillf Danton

Subject: Re: [PATCH] fix vulnerability of the release method of file operations in Block layer SCSI generic driver

On Wed, Nov 10, 2010 at 11:18 PM, Matthew Wilcox <[email protected]> wrote:
> On Wed, Nov 10, 2010 at 10:08:37PM +0800, Hillf Danton wrote:
>> The computation context setup by previous opening the bsg file could
>> not survive following open/release operations upon the same file
>> object.
>
> Umm .. release is called on final close of a file, not on every close
> of a file.
>
right, thanks//Hillf

>> The vulnerability is fixed by deferring the cleanup operation until necessary.
>>
>> Signed-off-by: Hillf Danton <[email protected]>
>> ---
>>
>> --- a/block/bsg.c     2010-09-13 07:07:38.000000000 +0800
>> +++ b/block/bsg.c     2010-11-10 21:43:58.000000000 +0800
>> @@ -858,7 +858,8 @@ static int bsg_release(struct inode *ino
>>  {
>>       struct bsg_device *bd = file->private_data;
>>
>> -     file->private_data = NULL;
>> +     if (1 == atomic_read(&bd->ref_count))
>> +             file->private_data = NULL;
>>       return bsg_put_device(bd);
>>  }
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to [email protected]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> Matthew Wilcox                          Intel Open Source Technology Centre
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours.  We can't possibly take such
> a retrograde step."
>

2010-12-09 13:49:55

by Hillf Danton

Subject: Re: [PATCH] fix vulnerability of the release method of file operations in Block layer SCSI generic driver

On Wed, Nov 10, 2010 at 11:18 PM, Matthew Wilcox <[email protected]> wrote:
> On Wed, Nov 10, 2010 at 10:08:37PM +0800, Hillf Danton wrote:
>> The computation context setup by previous opening the bsg file could
>> not survive following open/release operations upon the same file
>> object.
>
> Umm .. release is called on final close of a file, not on every close
> of a file.

Thanks for sharing the knowledge about the release callback.

But I am still wondering, if the operations in the release is safe, how the
increments, pumped up by open operations on cmdline, of the ref_count
of bsg device then get decreased, which could trigger mm leakage.

And both vulnerability and leakage, if possible, could be fixed by replacing
the only line we concerned with printk, see below please.

Cheers
Hillf
---

--- a/block/bsg.c 2010-11-01 19:54:12.000000000 +0800
+++ b/block/bsg.c 2010-12-09 21:38:32.000000000 +0800
@@ -855,7 +855,10 @@ static int bsg_release(struct inode *ino
{
struct bsg_device *bd = file->private_data;

- file->private_data = NULL;
+ if (atomic_read(&bd->ref_count) != 1)
+ printk(KERN_WARNING "bsg: ref count of %s is %d\n",
+ bd->name, atomic_read(&bd->ref_count));
+
return bsg_put_device(bd);
}