[PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[Chen Gang]

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 <gang.chen@asianux.com>
---
 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

Re: [PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[Kees Cook]

On Wed, Oct 30, 2013 at 7:52 PM, Chen Gang <gang.chen@asianux.com> 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 <gang.chen@asianux.com>
> ---
>  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

Re: [PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[Al Viro]

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?

Re: [PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[Kees Cook]

On Thu, Oct 31, 2013 at 12:06 PM, Al Viro <viro@zeniv.linux.org.uk> 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

Re: [PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[Greg KH]

On Thu, Oct 31, 2013 at 12:08:33PM -0700, Kees Cook wrote:
> On Thu, Oct 31, 2013 at 12:06 PM, Al Viro <viro@zeniv.linux.org.uk> 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

Re: [PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[Chen Gang]

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 <viro@zeniv.linux.org.uk> 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

Re: [PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[Chen Gang]

On 11/01/2013 10:41 AM, Chen Gang wrote:
> 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 <viro@zeniv.linux.org.uk> 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)".
> 

Oh, for me, it is not suitable to move a file system sub-directory to
"drivers/*/" sub-directory. And I can not find any sub-directory like
'staging' under "fs" sub-directory, either.

Do we have any sub-directory like "staging" in "fs" sub-directory? if
no, do we have to create it or have to use another ways instead of?


After reference the "drivers/staging" explanation (list below), we know
it is only for drivers (not fs).

menuconfig STAGING
        bool "Staging drivers"
        default n
        ---help---
          This option allows you to select a number of drivers that are
          not of the "normal" Linux kernel quality level.  These drivers
          are placed here in order to get a wider audience to make use of
          them.  Please note that these drivers are under heavy
          development, may or may not work, and may contain userspace
          interfaces that most likely will be changed in the near
          future.

          Using any of these drivers will taint your kernel which might
          affect support options from both the community, and various
          commercial support organizations.

          If you wish to work on these drivers, to help improve them, or
          to report problems you have with them, please see the
          driver_name.README file in the drivers/staging/ directory to
          see what needs to be worked on, and who to contact.

          If in doubt, say N here.


Thanks.

> If what I guess is correct, I support you (else, please let me know)
> 
> 
> Thanks.
> 

-- 
Chen Gang

Re: [PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[Greg KH]

On Sat, Nov 02, 2013 at 09:46:31PM +0800, Chen Gang wrote:
> On 11/01/2013 10:41 AM, Chen Gang wrote:
> > 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 <viro@zeniv.linux.org.uk> 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)".
> > 
> 
> Oh, for me, it is not suitable to move a file system sub-directory to
> "drivers/*/" sub-directory. And I can not find any sub-directory like
> 'staging' under "fs" sub-directory, either.
> 
> Do we have any sub-directory like "staging" in "fs" sub-directory? if
> no, do we have to create it or have to use another ways instead of?

Just move the filesystem to drivers/staging/befs.

Re: [PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[Al Viro]

On Sat, Nov 02, 2013 at 08:44:46AM -0700, Greg KH wrote:

> > Oh, for me, it is not suitable to move a file system sub-directory to
> > "drivers/*/" sub-directory. And I can not find any sub-directory like
> > 'staging' under "fs" sub-directory, either.
> > 
> > Do we have any sub-directory like "staging" in "fs" sub-directory? if
> > no, do we have to create it or have to use another ways instead of?
> 
> Just move the filesystem to drivers/staging/befs.

Actually, having read through that code...  It's not too scary; r/w
support would've been much more hairy, but this is just r/o.  We
probably don't need to move that sucker at all.

As for befs_get_block(), I'd suggest
	* taking the range checks for block number into its ->bmap()
(just check against the file size and return 0 if it doesn't fit)
	* turning the check for create != 0 into BUG_ON(create)
	* making the befs_fblock2brun() failure quiet - the sucker
will complain itself, just return that -EFBIG and be done with that.

Re: [PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[Chen Gang]

On 11/03/2013 12:27 AM, Al Viro wrote:
> On Sat, Nov 02, 2013 at 08:44:46AM -0700, Greg KH wrote:
> 
>>> Oh, for me, it is not suitable to move a file system sub-directory to
>>> "drivers/*/" sub-directory. And I can not find any sub-directory like
>>> 'staging' under "fs" sub-directory, either.
>>>
>>> Do we have any sub-directory like "staging" in "fs" sub-directory? if
>>> no, do we have to create it or have to use another ways instead of?
>>
>> Just move the filesystem to drivers/staging/befs.
> 
> Actually, having read through that code...  It's not too scary; r/w
> support would've been much more hairy, but this is just r/o.  We
> probably don't need to move that sucker at all.
> 

After a quick look, I guess we can not remove it from Linux kernel:

 - according to "fs/befs/Kconfig",
   it is only for support file system from BeOS.

 - according to "http://en.wikipedia.org/wiki/BeOS",
   BeOS is still in use (although it is almost dead).

 - according to "git log -p fs/befs",
   it is still be changed recently.

Can any members provide more proofs for it (my proofs are not enough).
if it is still using, we have to keep it in Linux kernel, else support
to remove it to some where firstly (but not "drivers/*").


> As for befs_get_block(), I'd suggest
> 	* taking the range checks for block number into its ->bmap()
> (just check against the file size and return 0 if it doesn't fit)

befs_get_block() is called both by befs_bmap() and by befs_readpage(),
and I guess Al's "->bmap()" points to befs_bmap() -- if what I guess is
incorrect, please let me know, thanks.

So for me, just remove this check "if (block < 0) { ... }" is OK which
is Kees said.

And for me, need use "%llu" not "%lu" instead of "%ld" (according to
"Documentation/printk-format.txt").


> 	* turning the check for create != 0 into BUG_ON(create)

Can we be sure of:

 - in current case, kernel/sub-system must be continuing incorrectly.
 - and if let it continue, it will cause critical issue.

If:

  can be sure of both, use BUG_ON().
  can be sure of 1st item, use WARN_ON().
  can be sure of neither, keep original no touch.

For me, if what Kees said is correct (for "ioctl ... printks ..."), I
support to use WARN_ON() which indicates "the sub-system is continuing
incorrectly, although it not a critical issue".


> 	* making the befs_fblock2brun() failure quiet - the sucker
> will complain itself, just return that -EFBIG and be done with that.
> 

Do you mean befs_fblock2brun() must be OK (should check it previously)?
If not, we cann't skip the failure -- next iaddr2blockno() may cause panic.

And for me, use "-EFBIG" is really not quite smart.

Hmm... the same reason as above (if "ioctl ... printks ..." correct),
may we need WARN_ON(), and then still return "-EFBIG"?.



BTW: the local variable 'res' can be removed.



Thanks.
-- 
Chen Gang