Need signed cast for it, the original author assume the type of 'block'
is long, so just cast to long. The related warnings (with allmodconfig):
fs/befs/linuxvfs.c:136:2: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
Signed-off-by: Chen Gang <[email protected]>
---
fs/befs/linuxvfs.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
index daa15d6..27e5179 100644
--- a/fs/befs/linuxvfs.c
+++ b/fs/befs/linuxvfs.c
@@ -133,7 +133,7 @@ befs_get_block(struct inode *inode, sector_t block,
befs_debug(sb, "---> befs_get_block() for inode %lu, block %ld",
inode->i_ino, block);
- if (block < 0) {
+ if ((long)block < 0) {
befs_error(sb, "befs_get_block() was asked for a block "
"number less than zero: block %ld in inode %lu",
block, inode->i_ino);
--
1.7.7.6
On Wed, Oct 30, 2013 at 7:52 PM, Chen Gang <[email protected]> wrote:
> Need signed cast for it, the original author assume the type of 'block'
> is long, so just cast to long. The related warnings (with allmodconfig):
>
> fs/befs/linuxvfs.c:136:2: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>
> Signed-off-by: Chen Gang <[email protected]>
> ---
> fs/befs/linuxvfs.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
> index daa15d6..27e5179 100644
> --- a/fs/befs/linuxvfs.c
> +++ b/fs/befs/linuxvfs.c
> @@ -133,7 +133,7 @@ befs_get_block(struct inode *inode, sector_t block,
> befs_debug(sb, "---> befs_get_block() for inode %lu, block %ld",
> inode->i_ino, block);
>
> - if (block < 0) {
> + if ((long)block < 0) {
> befs_error(sb, "befs_get_block() was asked for a block "
> "number less than zero: block %ld in inode %lu",
> block, inode->i_ino);
If block (type sector_t) is unsigned, we shouldn't cast it signed.
This entire code path should be removed. What is BEFS's expected
maximum block size? (Looks like even befs_blocknr_t is u64, so nothing
seems trivially in danger of wrapping.) I would also note that all the
format strings are wrong too (%ld instead of %lu).
-Kees
--
Kees Cook
Chrome OS Security
On Thu, Oct 31, 2013 at 09:53:59AM -0700, Kees Cook wrote:
> If block (type sector_t) is unsigned, we shouldn't cast it signed.
> This entire code path should be removed. What is BEFS's expected
> maximum block size? (Looks like even befs_blocknr_t is u64, so nothing
> seems trivially in danger of wrapping.) I would also note that all the
> format strings are wrong too (%ld instead of %lu).
FWIW, this
res = befs_fblock2brun(sb, ds, block, &run);
if (res != BEFS_OK) {
befs_error(sb,
"<--- befs_get_block() for inode %lu, block "
"%ld ERROR", inode->i_ino, block);
return -EFBIG;
}
also looks wrong - ioctl(..., FIBMAP, ...) shouldn't be able to spew
printks on a valid fs and hitting it with block number greater than
file length will, AFAICS, trigger that.
I agree that this code needs fixing, but just making gcc STFU about the
comparison would only serve to hide the problem. Anybody familiar with
befs or willing to learn it?
On Thu, Oct 31, 2013 at 12:06 PM, Al Viro <[email protected]> wrote:
> On Thu, Oct 31, 2013 at 09:53:59AM -0700, Kees Cook wrote:
>
>> If block (type sector_t) is unsigned, we shouldn't cast it signed.
>> This entire code path should be removed. What is BEFS's expected
>> maximum block size? (Looks like even befs_blocknr_t is u64, so nothing
>> seems trivially in danger of wrapping.) I would also note that all the
>> format strings are wrong too (%ld instead of %lu).
>
> FWIW, this
> res = befs_fblock2brun(sb, ds, block, &run);
> if (res != BEFS_OK) {
> befs_error(sb,
> "<--- befs_get_block() for inode %lu, block "
> "%ld ERROR", inode->i_ino, block);
> return -EFBIG;
> }
> also looks wrong - ioctl(..., FIBMAP, ...) shouldn't be able to spew
> printks on a valid fs and hitting it with block number greater than
> file length will, AFAICS, trigger that.
>
> I agree that this code needs fixing, but just making gcc STFU about the
> comparison would only serve to hide the problem. Anybody familiar with
> befs or willing to learn it?
Agreed. MAINTAINERS shows it as orphaned. Perhaps it should be moved
into staging?
-Kees
--
Kees Cook
Chrome OS Security
On Thu, Oct 31, 2013 at 12:08:33PM -0700, Kees Cook wrote:
> On Thu, Oct 31, 2013 at 12:06 PM, Al Viro <[email protected]> wrote:
> > On Thu, Oct 31, 2013 at 09:53:59AM -0700, Kees Cook wrote:
> >
> >> If block (type sector_t) is unsigned, we shouldn't cast it signed.
> >> This entire code path should be removed. What is BEFS's expected
> >> maximum block size? (Looks like even befs_blocknr_t is u64, so nothing
> >> seems trivially in danger of wrapping.) I would also note that all the
> >> format strings are wrong too (%ld instead of %lu).
> >
> > FWIW, this
> > res = befs_fblock2brun(sb, ds, block, &run);
> > if (res != BEFS_OK) {
> > befs_error(sb,
> > "<--- befs_get_block() for inode %lu, block "
> > "%ld ERROR", inode->i_ino, block);
> > return -EFBIG;
> > }
> > also looks wrong - ioctl(..., FIBMAP, ...) shouldn't be able to spew
> > printks on a valid fs and hitting it with block number greater than
> > file length will, AFAICS, trigger that.
> >
> > I agree that this code needs fixing, but just making gcc STFU about the
> > comparison would only serve to hide the problem. Anybody familiar with
> > befs or willing to learn it?
>
> Agreed. MAINTAINERS shows it as orphaned. Perhaps it should be moved
> into staging?
Only if we want to delete the thing. I'll be glad to take it there, and
remove it in 2 releases and then if anyone complains, we can add it back
easily. Just let me know.
thanks,
greg k-h
On 11/01/2013 04:45 AM, Greg KH wrote:
> On Thu, Oct 31, 2013 at 12:08:33PM -0700, Kees Cook wrote:
>> On Thu, Oct 31, 2013 at 12:06 PM, Al Viro <[email protected]> wrote:
>>> On Thu, Oct 31, 2013 at 09:53:59AM -0700, Kees Cook wrote:
>>>
>>>> If block (type sector_t) is unsigned, we shouldn't cast it signed.
>>>> This entire code path should be removed. What is BEFS's expected
>>>> maximum block size? (Looks like even befs_blocknr_t is u64, so nothing
>>>> seems trivially in danger of wrapping.) I would also note that all the
>>>> format strings are wrong too (%ld instead of %lu).
>>>
>>> FWIW, this
>>> res = befs_fblock2brun(sb, ds, block, &run);
>>> if (res != BEFS_OK) {
>>> befs_error(sb,
>>> "<--- befs_get_block() for inode %lu, block "
>>> "%ld ERROR", inode->i_ino, block);
>>> return -EFBIG;
>>> }
>>> also looks wrong - ioctl(..., FIBMAP, ...) shouldn't be able to spew
>>> printks on a valid fs and hitting it with block number greater than
>>> file length will, AFAICS, trigger that.
>>>
>>> I agree that this code needs fixing, but just making gcc STFU about the
>>> comparison would only serve to hide the problem. Anybody familiar with
>>> befs or willing to learn it?
>>
>> Agreed. MAINTAINERS shows it as orphaned. Perhaps it should be moved
>> into staging?
>
> Only if we want to delete the thing. I'll be glad to take it there, and
> remove it in 2 releases and then if anyone complains, we can add it back
> easily. Just let me know.
>
Excuse me, I am not quite familiar with BEFS, I guess your meaning is:
"if it is no further more discussion (e.g. within 1 week, no members reply), you will remove it (take it to "drivers/staging" sub-directory)".
If what I guess is correct, I support you (else, please let me know)
Thanks.
--
Chen Gang