ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block()
where we access the b_size of it. Since it is a local variable it
might contain some garbage. Make sure it is filled with zero before
passing.
Signed-off-by : Manish Katiyar <[email protected]>
---
fs/ext2/super.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index da8bdea..d10aa44 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct
super_block *sb, int type, char *data,
tocopy = sb->s_blocksize - offset < toread ?
sb->s_blocksize - offset : toread;
- tmp_bh.b_state = 0;
+ memset(&tmp_bh, 0, sizeof(struct buffer_head));
err = ext2_get_block(inode, blk, &tmp_bh, 0);
if (err < 0)
return err;
@@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct
super_block *sb, int type,
tocopy = sb->s_blocksize - offset < towrite ?
sb->s_blocksize - offset : towrite;
- tmp_bh.b_state = 0;
+ memset(&tmp_bh, 0, sizeof(struct buffer_head));
err = ext2_get_block(inode, blk, &tmp_bh, 1);
if (err < 0)
goto out;
--
1.5.4.3
Thanks -
Manish
Manish Katiyar wrote:
> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <[email protected]> wrote:
>> ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block()
>> where we access the b_size of it. Since it is a local variable it
>> might contain some garbage. Make sure it is filled with zero before
>> passing.
>
> Hi Ted/mingming,
>
> Any feedback on this ??
This looks ok to me, Manish. I'm curious, did you see this fail in real
life, and if so, what'd the failure look like?
With the change, the tmp_bh bh_size is 0, so maxblocks down the
get_block path is also 0, but I guess that works out ok.
-Eric
> Thanks -
> Manish
>
>> Signed-off-by : Manish Katiyar <[email protected]>
>> ---
>> fs/ext2/super.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
>> index da8bdea..d10aa44 100644
>> --- a/fs/ext2/super.c
>> +++ b/fs/ext2/super.c
>> @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct
>> super_block *sb, int type, char *data,
>> tocopy = sb->s_blocksize - offset < toread ?
>> sb->s_blocksize - offset : toread;
>>
>> - tmp_bh.b_state = 0;
>> + memset(&tmp_bh, 0, sizeof(struct buffer_head));
>> err = ext2_get_block(inode, blk, &tmp_bh, 0);
>> if (err < 0)
>> return err;
>> @@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct
>> super_block *sb, int type,
>> tocopy = sb->s_blocksize - offset < towrite ?
>> sb->s_blocksize - offset : towrite;
>>
>> - tmp_bh.b_state = 0;
>> + memset(&tmp_bh, 0, sizeof(struct buffer_head));
>> err = ext2_get_block(inode, blk, &tmp_bh, 1);
>> if (err < 0)
>> goto out;
>> --
>> 1.5.4.3
>>
>>
>> Thanks -
>> Manish
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 25, 2009 at 9:45 PM, Eric Sandeen <[email protected]> wrote:
> Manish Katiyar wrote:
>> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <[email protected]> wrote:
>>> ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block()
>>> where we access the b_size of it. Since it is a local variable it
>>> might contain some garbage. Make sure it is filled with zero before
>>> passing.
>>
>> Hi Ted/mingming,
>>
>> Any feedback on this ??
>
> This looks ok to me, Manish. I'm curious, did you see this fail in real
> life, and if so, what'd the failure look like?
Actually no......I realised this while going through the code. I was
also wondering why we haven't hit this till now. Since ext{3,4} don't
have this issue, the only reason I can think of is because ext2 with
quota is not very much used or somehow we are lucky.
>
> With the change, the tmp_bh bh_size is 0, so maxblocks down the
> get_block path is also 0, but I guess that works out ok.
Yes, but that is better than having a random garbage. Isn't it ?
Thanks -
Manish
>
> -Eric
>
>> Thanks -
>> Manish
>>
>>> Signed-off-by : Manish Katiyar <[email protected]>
>>> ---
>>> fs/ext2/super.c | 4 ++--
>>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
>>> index da8bdea..d10aa44 100644
>>> --- a/fs/ext2/super.c
>>> +++ b/fs/ext2/super.c
>>> @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct
>>> super_block *sb, int type, char *data,
>>> tocopy = sb->s_blocksize - offset < toread ?
>>> sb->s_blocksize - offset : toread;
>>>
>>> - tmp_bh.b_state = 0;
>>> + memset(&tmp_bh, 0, sizeof(struct buffer_head));
>>> err = ext2_get_block(inode, blk, &tmp_bh, 0);
>>> if (err < 0)
>>> return err;
>>> @@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct
>>> super_block *sb, int type,
>>> tocopy = sb->s_blocksize - offset < towrite ?
>>> sb->s_blocksize - offset : towrite;
>>>
>>> - tmp_bh.b_state = 0;
>>> + memset(&tmp_bh, 0, sizeof(struct buffer_head));
>>> err = ext2_get_block(inode, blk, &tmp_bh, 1);
>>> if (err < 0)
>>> goto out;
>>> --
>>> 1.5.4.3
>>>
>>>
>>> Thanks -
>>> Manish
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <[email protected]> wrote:
> ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block()
> where we access the b_size of it. Since it is a local variable it
> might contain some garbage. Make sure it is filled with zero before
> passing.
Hi Ted/mingming,
Any feedback on this ??
Thanks -
Manish
>
> Signed-off-by : Manish Katiyar <[email protected]>
> ---
> fs/ext2/super.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index da8bdea..d10aa44 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct
> super_block *sb, int type, char *data,
> tocopy = sb->s_blocksize - offset < toread ?
> sb->s_blocksize - offset : toread;
>
> - tmp_bh.b_state = 0;
> + memset(&tmp_bh, 0, sizeof(struct buffer_head));
> err = ext2_get_block(inode, blk, &tmp_bh, 0);
> if (err < 0)
> return err;
> @@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct
> super_block *sb, int type,
> tocopy = sb->s_blocksize - offset < towrite ?
> sb->s_blocksize - offset : towrite;
>
> - tmp_bh.b_state = 0;
> + memset(&tmp_bh, 0, sizeof(struct buffer_head));
> err = ext2_get_block(inode, blk, &tmp_bh, 1);
> if (err < 0)
> goto out;
> --
> 1.5.4.3
>
>
> Thanks -
> Manish
>
Manish Katiyar wrote:
> On Sun, Jan 25, 2009 at 9:45 PM, Eric Sandeen <[email protected]> wrote:
>> Manish Katiyar wrote:
>>> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <[email protected]> wrote:
>>>> ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block()
>>>> where we access the b_size of it. Since it is a local variable it
>>>> might contain some garbage. Make sure it is filled with zero before
>>>> passing.
>>> Hi Ted/mingming,
>>>
>>> Any feedback on this ??
>> This looks ok to me, Manish. I'm curious, did you see this fail in real
>> life, and if so, what'd the failure look like?
>
> Actually no......I realised this while going through the code. I was
> also wondering why we haven't hit this till now. Since ext{3,4} don't
> have this issue, the only reason I can think of is because ext2 with
> quota is not very much used or somehow we are lucky.
>
>> With the change, the tmp_bh bh_size is 0, so maxblocks down the
>> get_block path is also 0, but I guess that works out ok.
>
> Yes, but that is better than having a random garbage. Isn't it ?
Absolutely; it just struck me as a little odd but it's exactly what
__ext2_get_block does as well, I probably just need to take a closer
look at what it does w/ a 0 max.
-Eric
> Thanks -
> Manish
> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <[email protected]> wrote:
> > ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block()
> > where we access the b_size of it. Since it is a local variable it
> > might contain some garbage. Make sure it is filled with zero before
> > passing.
>
> Hi Ted/mingming,
>
> Any feedback on this ??
Ops, sorry. I wanted to reply but first I wanted to research more
whether we can set b_size to 0 and then forgot about it. Looking into
other code (e.g. in fs/mpage.c or fs/buffer.c) I think it would be
better to set b_size to sb->s_blocksize and be done with that. Mapping
code does not need anything else set to a deterministic value so using
memset is a bit overkill.
Honza
> > Signed-off-by : Manish Katiyar <[email protected]>
> > ---
> > fs/ext2/super.c | 4 ++--
> > 1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> > index da8bdea..d10aa44 100644
> > --- a/fs/ext2/super.c
> > +++ b/fs/ext2/super.c
> > @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct
> > super_block *sb, int type, char *data,
> > tocopy = sb->s_blocksize - offset < toread ?
> > sb->s_blocksize - offset : toread;
> >
> > - tmp_bh.b_state = 0;
> > + memset(&tmp_bh, 0, sizeof(struct buffer_head));
> > err = ext2_get_block(inode, blk, &tmp_bh, 0);
> > if (err < 0)
> > return err;
> > @@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct
> > super_block *sb, int type,
> > tocopy = sb->s_blocksize - offset < towrite ?
> > sb->s_blocksize - offset : towrite;
> >
> > - tmp_bh.b_state = 0;
> > + memset(&tmp_bh, 0, sizeof(struct buffer_head));
> > err = ext2_get_block(inode, blk, &tmp_bh, 1);
> > if (err < 0)
> > goto out;
> > --
> > 1.5.4.3
> >
> >
> > Thanks -
> > Manish
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Kara <[email protected]>
SuSE CR Labs
> On Sun, Jan 25, 2009 at 9:45 PM, Eric Sandeen <[email protected]> wrote:
> > Manish Katiyar wrote:
> >> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <[email protected]> wrote:
> >>> ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block()
> >>> where we access the b_size of it. Since it is a local variable it
> >>> might contain some garbage. Make sure it is filled with zero before
> >>> passing.
> >>
> >> Hi Ted/mingming,
> >>
> >> Any feedback on this ??
> >
> > This looks ok to me, Manish. I'm curious, did you see this fail in real
> > life, and if so, what'd the failure look like?
>
> Actually no......I realised this while going through the code. I was
> also wondering why we haven't hit this till now. Since ext{3,4} don't
> have this issue, the only reason I can think of is because ext2 with
> quota is not very much used or somehow we are lucky.
Well, I think ext2 with quotas is not used very often. But also note
that if a random garbage is passed in it has high chances that maxblocks
is >= 1. And that is all what is needed for ext2_get_blocks() to return
what we want.
Honza
--
Jan Kara <[email protected]>
SuSE CR Labs
On Mon, Jan 26, 2009 at 5:59 PM, Jan Kara <[email protected]> wrote:
>> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <[email protected]> wrote:
>> > ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block()
>> > where we access the b_size of it. Since it is a local variable it
>> > might contain some garbage. Make sure it is filled with zero before
>> > passing.
>>
>> Hi Ted/mingming,
>>
>> Any feedback on this ??
> Ops, sorry. I wanted to reply but first I wanted to research more
> whether we can set b_size to 0 and then forgot about it. Looking into
> other code (e.g. in fs/mpage.c or fs/buffer.c) I think it would be
> better to set b_size to sb->s_blocksize and be done with that. Mapping
> code does not need anything else set to a deterministic value so using
> memset is a bit overkill.
Thanks Jan for your comments. Yes memset is an overkill. I did it just
because other users of ext2_get_block were doing the same way. Will
rework the patch with setting b_size as blocksize and send again.
Thanks -
Manish
>
> Honza
>
>> > Signed-off-by : Manish Katiyar <[email protected]>
>> > ---
>> > fs/ext2/super.c | 4 ++--
>> > 1 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/fs/ext2/super.c b/fs/ext2/super.c
>> > index da8bdea..d10aa44 100644
>> > --- a/fs/ext2/super.c
>> > +++ b/fs/ext2/super.c
>> > @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct
>> > super_block *sb, int type, char *data,
>> > tocopy = sb->s_blocksize - offset < toread ?
>> > sb->s_blocksize - offset : toread;
>> >
>> > - tmp_bh.b_state = 0;
>> > + memset(&tmp_bh, 0, sizeof(struct buffer_head));
>> > err = ext2_get_block(inode, blk, &tmp_bh, 0);
>> > if (err < 0)
>> > return err;
>> > @@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct
>> > super_block *sb, int type,
>> > tocopy = sb->s_blocksize - offset < towrite ?
>> > sb->s_blocksize - offset : towrite;
>> >
>> > - tmp_bh.b_state = 0;
>> > + memset(&tmp_bh, 0, sizeof(struct buffer_head));
>> > err = ext2_get_block(inode, blk, &tmp_bh, 1);
>> > if (err < 0)
>> > goto out;
>> > --
>> > 1.5.4.3
>> >
>> >
>> > Thanks -
>> > Manish
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> Jan Kara <[email protected]>
> SuSE CR Labs
>
On Mon, Jan 26, 2009 at 6:03 PM, Manish Katiyar <[email protected]> wrote:
> On Mon, Jan 26, 2009 at 5:59 PM, Jan Kara <[email protected]> wrote:
>>> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <[email protected]> wrote:
>>> > ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block()
>>> > where we access the b_size of it. Since it is a local variable it
>>> > might contain some garbage. Make sure it is filled with zero before
>>> > passing.
>>>
>>> Hi Ted/mingming,
>>>
>>> Any feedback on this ??
>> Ops, sorry. I wanted to reply but first I wanted to research more
>> whether we can set b_size to 0 and then forgot about it. Looking into
>> other code (e.g. in fs/mpage.c or fs/buffer.c) I think it would be
>> better to set b_size to sb->s_blocksize and be done with that. Mapping
>> code does not need anything else set to a deterministic value so using
>> memset is a bit overkill.
>
> Thanks Jan for your comments. Yes memset is an overkill. I did it just
> because other users of ext2_get_block were doing the same way. Will
> rework the patch with setting b_size as blocksize and send again.
Here is the updated patch. compile tested.
Signed-off-by : Manish Katiyar <[email protected]>
---
fs/ext2/super.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index da8bdea..2a1c0c6 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -1328,6 +1328,7 @@ static ssize_t ext2_quota_read(struct
super_block *sb, int type, char *data,
sb->s_blocksize - offset : toread;
tmp_bh.b_state = 0;
+ tmp_bh.b_size = sb->s_blocksize;
err = ext2_get_block(inode, blk, &tmp_bh, 0);
if (err < 0)
return err;
@@ -1367,6 +1368,7 @@ static ssize_t ext2_quota_write(struct
super_block *sb, int type,
sb->s_blocksize - offset : towrite;
tmp_bh.b_state = 0;
+ tmp_bh.b_size = sb->s_blocksize;
err = ext2_get_block(inode, blk, &tmp_bh, 1);
if (err < 0)
goto out;
--
1.5.4.3
Thanks -
Manish
>
> Thanks -
> Manish
>
>>
>> Honza
>>
>>> > Signed-off-by : Manish Katiyar <[email protected]>
>>> > ---
>>> > fs/ext2/super.c | 4 ++--
>>> > 1 files changed, 2 insertions(+), 2 deletions(-)
>>> >
>>> > diff --git a/fs/ext2/super.c b/fs/ext2/super.c
>>> > index da8bdea..d10aa44 100644
>>> > --- a/fs/ext2/super.c
>>> > +++ b/fs/ext2/super.c
>>> > @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct
>>> > super_block *sb, int type, char *data,
>>> > tocopy = sb->s_blocksize - offset < toread ?
>>> > sb->s_blocksize - offset : toread;
>>> >
>>> > - tmp_bh.b_state = 0;
>>> > + memset(&tmp_bh, 0, sizeof(struct buffer_head));
>>> > err = ext2_get_block(inode, blk, &tmp_bh, 0);
>>> > if (err < 0)
>>> > return err;
>>> > @@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct
>>> > super_block *sb, int type,
>>> > tocopy = sb->s_blocksize - offset < towrite ?
>>> > sb->s_blocksize - offset : towrite;
>>> >
>>> > - tmp_bh.b_state = 0;
>>> > + memset(&tmp_bh, 0, sizeof(struct buffer_head));
>>> > err = ext2_get_block(inode, blk, &tmp_bh, 1);
>>> > if (err < 0)
>>> > goto out;
>>> > --
>>> > 1.5.4.3
>>> >
>>> >
>>> > Thanks -
>>> > Manish
>>> >
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> --
>> Jan Kara <[email protected]>
>> SuSE CR Labs
>>
>
On Mon 26-01-09 18:18:19, Manish Katiyar wrote:
> On Mon, Jan 26, 2009 at 6:03 PM, Manish Katiyar <[email protected]> wrote:
> > On Mon, Jan 26, 2009 at 5:59 PM, Jan Kara <[email protected]> wrote:
> >>> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <[email protected]> wrote:
> >>> > ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block()
> >>> > where we access the b_size of it. Since it is a local variable it
> >>> > might contain some garbage. Make sure it is filled with zero before
> >>> > passing.
> >>>
> >>> Hi Ted/mingming,
> >>>
> >>> Any feedback on this ??
> >> Ops, sorry. I wanted to reply but first I wanted to research more
> >> whether we can set b_size to 0 and then forgot about it. Looking into
> >> other code (e.g. in fs/mpage.c or fs/buffer.c) I think it would be
> >> better to set b_size to sb->s_blocksize and be done with that. Mapping
> >> code does not need anything else set to a deterministic value so using
> >> memset is a bit overkill.
> >
> > Thanks Jan for your comments. Yes memset is an overkill. I did it just
> > because other users of ext2_get_block were doing the same way. Will
> > rework the patch with setting b_size as blocksize and send again.
>
> Here is the updated patch. compile tested.
>
> Signed-off-by : Manish Katiyar <[email protected]>
Yes, now it looks fine. Thanks for the fix.
Acked-by: Jan Kara <[email protected]>
Honza
> ---
> fs/ext2/super.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index da8bdea..2a1c0c6 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -1328,6 +1328,7 @@ static ssize_t ext2_quota_read(struct
> super_block *sb, int type, char *data,
> sb->s_blocksize - offset : toread;
>
> tmp_bh.b_state = 0;
> + tmp_bh.b_size = sb->s_blocksize;
> err = ext2_get_block(inode, blk, &tmp_bh, 0);
> if (err < 0)
> return err;
> @@ -1367,6 +1368,7 @@ static ssize_t ext2_quota_write(struct
> super_block *sb, int type,
> sb->s_blocksize - offset : towrite;
>
> tmp_bh.b_state = 0;
> + tmp_bh.b_size = sb->s_blocksize;
> err = ext2_get_block(inode, blk, &tmp_bh, 1);
> if (err < 0)
> goto out;
> --
> 1.5.4.3
--
Jan Kara <[email protected]>
SUSE Labs, CR