[PATCH] vboxsf: fix old signature detection

[PATCH] vboxsf: fix old signature detection

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The constant-out-of-range check in clang found an actual bug in
vboxsf, which leads to the detection of old mount signatures always
failing:

fs/vboxsf/super.c:394:21: error: result of comparison of constant -3 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
                       options[3] == VBSF_MOUNT_SIGNATURE_BYTE_3) {
                       ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~
fs/vboxsf/super.c:393:21: error: result of comparison of constant -2 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
                       options[2] == VBSF_MOUNT_SIGNATURE_BYTE_2 &&
                       ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~
fs/vboxsf/super.c:392:21: error: result of comparison of constant -1 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
                       options[1] == VBSF_MOUNT_SIGNATURE_BYTE_1 &&
                       ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~

The problem is that the pointer is of type 'unsigned char' but the
constant is a 'char'. My first idea was to change the type of the
pointer to 'char *', but I noticed that this was the original code
and it got changed after 'smatch' complained about this.

I don't know if there is a bug in smatch here, but it sounds to me
that clang's warning is correct. Forcing the constants to an unsigned
type should make the code behave consistently and avoid the warning
on both.

Fixes: 9d682ea6bcc7 ("vboxsf: Fix the check for the old binary mount-arguments struct")
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/vboxsf/super.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/vboxsf/super.c b/fs/vboxsf/super.c
index 4f5e59f06284..84e2236021de 100644
--- a/fs/vboxsf/super.c
+++ b/fs/vboxsf/super.c
@@ -21,10 +21,10 @@
 
 #define VBOXSF_SUPER_MAGIC 0x786f4256 /* 'VBox' little endian */
 
-#define VBSF_MOUNT_SIGNATURE_BYTE_0 ('\000')
-#define VBSF_MOUNT_SIGNATURE_BYTE_1 ('\377')
-#define VBSF_MOUNT_SIGNATURE_BYTE_2 ('\376')
-#define VBSF_MOUNT_SIGNATURE_BYTE_3 ('\375')
+#define VBSF_MOUNT_SIGNATURE_BYTE_0 (u8)('\000')
+#define VBSF_MOUNT_SIGNATURE_BYTE_1 (u8)('\377')
+#define VBSF_MOUNT_SIGNATURE_BYTE_2 (u8)('\376')
+#define VBSF_MOUNT_SIGNATURE_BYTE_3 (u8)('\375')
 
 static int follow_symlinks;
 module_param(follow_symlinks, int, 0444);
-- 
2.29.2

Re: [PATCH] vboxsf: fix old signature detection

[Hans de Goede]

Hi,

On 9/27/21 11:40 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The constant-out-of-range check in clang found an actual bug in
> vboxsf, which leads to the detection of old mount signatures always
> failing:
> 
> fs/vboxsf/super.c:394:21: error: result of comparison of constant -3 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
>                        options[3] == VBSF_MOUNT_SIGNATURE_BYTE_3) {
>                        ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~

This actually seems to be a clang bug though, or at least a weird
interpretation (and different from gcc) of the C spec.

VBSF_MOUNT_SIGNATURE_BYTE_3 is defined as:

#define VBSF_MOUNT_SIGNATURE_BYTE_3 ('\375')

The C-spec:

http://port70.net/~nsz/c/c11/n1570.html#6.4.4.4p5

Says the following:

"The octal digits that follow the backslash in an octal escape sequence are taken to be part of the construction of a single character for an integer character constant or of a single wide character for a wide character constant. The numerical value of the octal integer so formed specifies the value of the desired character or wide character."

Character constants have a type of int, so 0375
clearly fits in the range of that.

I guess the problem is that gcc sees this as

const int VBSF_MOUNT_SIGNATURE_BYTE_3 = 0375;

Where as clang sees this as:

const int VBSF_MOUNT_SIGNATURE_BYTE_3 = (char)0375;

Which is a nice subtle incompatibility between the 2 :|


With that said, the patch is fine and I have no objections
against it:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Although maybe it is better to actually remove any
ambiguity and just replace the defines with:

static const u8 VBSF_MOUNT_SIGNATURE_BYTE_0 = 0000;
static const u8 VBSF_MOUNT_SIGNATURE_BYTE_1 = 0377;
static const u8 VBSF_MOUNT_SIGNATURE_BYTE_2 = 0376;
static const u8 VBSF_MOUNT_SIGNATURE_BYTE_3 = 0375;

?

Regards,

Hans










> fs/vboxsf/super.c:393:21: error: result of comparison of constant -2 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
>                        options[2] == VBSF_MOUNT_SIGNATURE_BYTE_2 &&
>                        ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> fs/vboxsf/super.c:392:21: error: result of comparison of constant -1 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
>                        options[1] == VBSF_MOUNT_SIGNATURE_BYTE_1 &&
>                        ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> The problem is that the pointer is of type 'unsigned char' but the
> constant is a 'char'. My first idea was to change the type of the
> pointer to 'char *', but I noticed that this was the original code
> and it got changed after 'smatch' complained about this.
> 
> I don't know if there is a bug in smatch here, but it sounds to me
> that clang's warning is correct. Forcing the constants to an unsigned
> type should make the code behave consistently and avoid the warning
> on both.
> 
> Fixes: 9d682ea6bcc7 ("vboxsf: Fix the check for the old binary mount-arguments struct")
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  fs/vboxsf/super.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/vboxsf/super.c b/fs/vboxsf/super.c
> index 4f5e59f06284..84e2236021de 100644
> --- a/fs/vboxsf/super.c
> +++ b/fs/vboxsf/super.c
> @@ -21,10 +21,10 @@
>  
>  #define VBOXSF_SUPER_MAGIC 0x786f4256 /* 'VBox' little endian */
>  
> -#define VBSF_MOUNT_SIGNATURE_BYTE_0 ('\000')
> -#define VBSF_MOUNT_SIGNATURE_BYTE_1 ('\377')
> -#define VBSF_MOUNT_SIGNATURE_BYTE_2 ('\376')
> -#define VBSF_MOUNT_SIGNATURE_BYTE_3 ('\375')
> +#define VBSF_MOUNT_SIGNATURE_BYTE_0 (u8)('\000')
> +#define VBSF_MOUNT_SIGNATURE_BYTE_1 (u8)('\377')
> +#define VBSF_MOUNT_SIGNATURE_BYTE_2 (u8)('\376')
> +#define VBSF_MOUNT_SIGNATURE_BYTE_3 (u8)('\375')
>  
>  static int follow_symlinks;
>  module_param(follow_symlinks, int, 0444);
> 

Re: [PATCH] vboxsf: fix old signature detection

[Dan Carpenter]

GCC handles it the same way as Clang.  '\377' is -1 but in Sparse it's
255.  I've added the Sparse mailing list to the CC.

regards,
dan carpenter

On Mon, Sep 27, 2021 at 12:09:01PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 9/27/21 11:40 AM, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > The constant-out-of-range check in clang found an actual bug in
> > vboxsf, which leads to the detection of old mount signatures always
> > failing:
> > 
> > fs/vboxsf/super.c:394:21: error: result of comparison of constant -3 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> >                        options[3] == VBSF_MOUNT_SIGNATURE_BYTE_3) {
> >                        ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This actually seems to be a clang bug though, or at least a weird
> interpretation (and different from gcc) of the C spec.
> 
> VBSF_MOUNT_SIGNATURE_BYTE_3 is defined as:
> 
> #define VBSF_MOUNT_SIGNATURE_BYTE_3 ('\375')
> 
> The C-spec:
> 
> http://port70.net/~nsz/c/c11/n1570.html#6.4.4.4p5 
> 
> Says the following:
> 
> "The octal digits that follow the backslash in an octal escape sequence are taken to be part of the construction of a single character for an integer character constant or of a single wide character for a wide character constant. The numerical value of the octal integer so formed specifies the value of the desired character or wide character."
> 
> Character constants have a type of int, so 0375
> clearly fits in the range of that.
> 
> I guess the problem is that gcc sees this as
> 
> const int VBSF_MOUNT_SIGNATURE_BYTE_3 = 0375;
> 
> Where as clang sees this as:
> 
> const int VBSF_MOUNT_SIGNATURE_BYTE_3 = (char)0375;
> 
> Which is a nice subtle incompatibility between the 2 :|
> 
> 
> With that said, the patch is fine and I have no objections
> against it:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Although maybe it is better to actually remove any
> ambiguity and just replace the defines with:
> 
> static const u8 VBSF_MOUNT_SIGNATURE_BYTE_0 = 0000;
> static const u8 VBSF_MOUNT_SIGNATURE_BYTE_1 = 0377;
> static const u8 VBSF_MOUNT_SIGNATURE_BYTE_2 = 0376;
> static const u8 VBSF_MOUNT_SIGNATURE_BYTE_3 = 0375;
> 
> ?
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> > fs/vboxsf/super.c:393:21: error: result of comparison of constant -2 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> >                        options[2] == VBSF_MOUNT_SIGNATURE_BYTE_2 &&
> >                        ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > fs/vboxsf/super.c:392:21: error: result of comparison of constant -1 with expression of type 'unsigned char' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> >                        options[1] == VBSF_MOUNT_SIGNATURE_BYTE_1 &&
> >                        ~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > The problem is that the pointer is of type 'unsigned char' but the
> > constant is a 'char'. My first idea was to change the type of the
> > pointer to 'char *', but I noticed that this was the original code
> > and it got changed after 'smatch' complained about this.
> > 
> > I don't know if there is a bug in smatch here, but it sounds to me
> > that clang's warning is correct. Forcing the constants to an unsigned
> > type should make the code behave consistently and avoid the warning
> > on both.
> > 
> > Fixes: 9d682ea6bcc7 ("vboxsf: Fix the check for the old binary mount-arguments struct")
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  fs/vboxsf/super.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/vboxsf/super.c b/fs/vboxsf/super.c
> > index 4f5e59f06284..84e2236021de 100644
> > --- a/fs/vboxsf/super.c
> > +++ b/fs/vboxsf/super.c
> > @@ -21,10 +21,10 @@
> >  
> >  #define VBOXSF_SUPER_MAGIC 0x786f4256 /* 'VBox' little endian */
> >  
> > -#define VBSF_MOUNT_SIGNATURE_BYTE_0 ('\000')
> > -#define VBSF_MOUNT_SIGNATURE_BYTE_1 ('\377')
> > -#define VBSF_MOUNT_SIGNATURE_BYTE_2 ('\376')
> > -#define VBSF_MOUNT_SIGNATURE_BYTE_3 ('\375')
> > +#define VBSF_MOUNT_SIGNATURE_BYTE_0 (u8)('\000')
> > +#define VBSF_MOUNT_SIGNATURE_BYTE_1 (u8)('\377')
> > +#define VBSF_MOUNT_SIGNATURE_BYTE_2 (u8)('\376')
> > +#define VBSF_MOUNT_SIGNATURE_BYTE_3 (u8)('\375')
> >  
> >  static int follow_symlinks;
> >  module_param(follow_symlinks, int, 0444);
> > 

Re: [PATCH] vboxsf: fix old signature detection

[Arnd Bergmann]

On Mon, Sep 27, 2021 at 3:02 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> GCC handles it the same way as Clang.  '\377' is -1 but in Sparse it's
> 255.  I've added the Sparse mailing list to the CC.

More specifically, ' think '\377' may be either -1 or 255 depending on
the architecture.
On most architectures, 'char' is implicitly signed, but on some others
it is not.

The original code before 9d682ea6bcc7 should have worked either way because
both sides of the comparison were the same 'char' type, marking one of them
as explicitly 'unsigned char' seems to have broken all architectures on which
the type is implicitly 'signed'.

       Arnd

Re: [PATCH] vboxsf: fix old signature detection

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 952 bytes --]

On Mon, Sep 27, 2021 at 6:22 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> More specifically, ' think '\377' may be either -1 or 255 depending on
> the architecture.
> On most architectures, 'char' is implicitly signed, but on some others
> it is not.

Yeah. That code is just broken.

And Arnd, your patch may be "conceptually minimal", in that it keeps
thed broken code and makes it work. But it just dials up the oddity to
11.

The proper patch is just this appended thing that stops playing silly
games, and just uses "memcmp()".

I've verified that with sane build configurations, it just generates

        testq   %rsi, %rsi
        je      .L25
        cmpl    $-33620224, (%rsi)
        je      .L31

for that

        if (data && !memcmp(data, VBSF_MOUNT_SIGNATURE, 4)) {

test. With a lot of crazy debug options you'll actually see the
"memcmp()", but the bad code generation is the least of your options
in that case.

               Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1239 bytes --]

 fs/vboxsf/super.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/fs/vboxsf/super.c b/fs/vboxsf/super.c
index 4f5e59f06284..37dd3fe5b1e9 100644
--- a/fs/vboxsf/super.c
+++ b/fs/vboxsf/super.c
@@ -21,10 +21,7 @@
 
 #define VBOXSF_SUPER_MAGIC 0x786f4256 /* 'VBox' little endian */
 
-#define VBSF_MOUNT_SIGNATURE_BYTE_0 ('\000')
-#define VBSF_MOUNT_SIGNATURE_BYTE_1 ('\377')
-#define VBSF_MOUNT_SIGNATURE_BYTE_2 ('\376')
-#define VBSF_MOUNT_SIGNATURE_BYTE_3 ('\375')
+static const unsigned char VBSF_MOUNT_SIGNATURE[4] = "\000\377\376\375";
 
 static int follow_symlinks;
 module_param(follow_symlinks, int, 0444);
@@ -386,12 +383,7 @@ static int vboxsf_setup(void)
 
 static int vboxsf_parse_monolithic(struct fs_context *fc, void *data)
 {
-	unsigned char *options = data;
-
-	if (options && options[0] == VBSF_MOUNT_SIGNATURE_BYTE_0 &&
-		       options[1] == VBSF_MOUNT_SIGNATURE_BYTE_1 &&
-		       options[2] == VBSF_MOUNT_SIGNATURE_BYTE_2 &&
-		       options[3] == VBSF_MOUNT_SIGNATURE_BYTE_3) {
+	if (data && !memcmp(data, VBSF_MOUNT_SIGNATURE, 4)) {
 		vbg_err("vboxsf: Old binary mount data not supported, remove obsolete mount.vboxsf and/or update your VBoxService.\n");
 		return -EINVAL;
 	}

Re: [PATCH] vboxsf: fix old signature detection

[Hans de Goede]

Hi Linus,

On 9/27/21 8:33 PM, Linus Torvalds wrote:
> On Mon, Sep 27, 2021 at 6:22 AM Arnd Bergmann <arnd@kernel.org> wrote:
>>
>> More specifically, ' think '\377' may be either -1 or 255 depending on
>> the architecture.
>> On most architectures, 'char' is implicitly signed, but on some others
>> it is not.
> 
> Yeah. That code is just broken.
> 
> And Arnd, your patch may be "conceptually minimal", in that it keeps
> thed broken code and makes it work. But it just dials up the oddity to
> 11.
> 
> The proper patch is just this appended thing that stops playing silly
> games, and just uses "memcmp()".
> 
> I've verified that with sane build configurations, it just generates
> 
>         testq   %rsi, %rsi
>         je      .L25
>         cmpl    $-33620224, (%rsi)
>         je      .L31
> 
> for that
> 
>         if (data && !memcmp(data, VBSF_MOUNT_SIGNATURE, 4)) {
> 
> test. With a lot of crazy debug options you'll actually see the
> "memcmp()", but the bad code generation is the least of your options
> in that case.

I agree that your suggestion is to be the best solution,
so how do we move forward with this, do I turn this into a
proper patch with you as the author and Arnd as Reported-by and
if yes may I add your Signed-off-by to the patch ?

Or do I make myself author and set you as Suggested-by ?

Regards,

Hans

Re: [PATCH] vboxsf: fix old signature detection

[Arnd Bergmann]

On Tue, Sep 28, 2021 at 11:40 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 9/27/21 8:33 PM, Linus Torvalds wrote:
> > On Mon, Sep 27, 2021 at 6:22 AM Arnd Bergmann <arnd@kernel.org> wrote:
> >>
> >> More specifically, ' think '\377' may be either -1 or 255 depending on
> >> the architecture.
> >> On most architectures, 'char' is implicitly signed, but on some others
> >> it is not.
> >
> > Yeah. That code is just broken.
> >
> > And Arnd, your patch may be "conceptually minimal", in that it keeps
> > thed broken code and makes it work. But it just dials up the oddity to
> > 11.

Thank you for addressing it. I usually try to avoid overthinking changes
to "unusual" code like this, but your solution is clearly an improvement.

What really threw me off this time is that my first attempt to address
the warning was an exact revert of 9d682ea6bcc7 ("vboxsf: Fix the
check for the old binary mount-arguments struct"), which in turn
came from a tool that is usually correct and and that both Dan
and Al thought the original patch was correct when it looked like
it turned a working (though unusual) implementation  into a broken
one.

> I agree that your suggestion is to be the best solution,
> so how do we move forward with this, do I turn this into a
> proper patch with you as the author and Arnd as Reported-by and
> if yes may I add your Signed-off-by to the patch ?

It's already upstream, see d5f6545934c4 ("qnx4: work around gcc
false positive warning bug").

      Arnd

Re: [PATCH] vboxsf: fix old signature detection

[Hans de Goede]

Hi,

On 9/28/21 12:11 PM, Arnd Bergmann wrote:
> On Tue, Sep 28, 2021 at 11:40 AM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 9/27/21 8:33 PM, Linus Torvalds wrote:
>>> On Mon, Sep 27, 2021 at 6:22 AM Arnd Bergmann <arnd@kernel.org> wrote:
>>>>
>>>> More specifically, ' think '\377' may be either -1 or 255 depending on
>>>> the architecture.
>>>> On most architectures, 'char' is implicitly signed, but on some others
>>>> it is not.
>>>
>>> Yeah. That code is just broken.
>>>
>>> And Arnd, your patch may be "conceptually minimal", in that it keeps
>>> thed broken code and makes it work. But it just dials up the oddity to
>>> 11.
> 
> Thank you for addressing it. I usually try to avoid overthinking changes
> to "unusual" code like this, but your solution is clearly an improvement.
> 
> What really threw me off this time is that my first attempt to address
> the warning was an exact revert of 9d682ea6bcc7 ("vboxsf: Fix the
> check for the old binary mount-arguments struct"), which in turn
> came from a tool that is usually correct and and that both Dan
> and Al thought the original patch was correct when it looked like
> it turned a working (though unusual) implementation  into a broken
> one.
> 
>> I agree that your suggestion is to be the best solution,
>> so how do we move forward with this, do I turn this into a
>> proper patch with you as the author and Arnd as Reported-by and
>> if yes may I add your Signed-off-by to the patch ?
> 
> It's already upstream, see d5f6545934c4 ("qnx4: work around gcc
> false positive warning bug").

Ah, actually you mean: 9b3b353ef330 ("vboxfs: fix broken legacy mount
signature checking"), but other then that yes you're right it
is already upstream.

Thank you Arnd and thank you Linus.

Regards,

Hans

Re: [PATCH] vboxsf: fix old signature detection

[Arnd Bergmann]

On Tue, Sep 28, 2021 at 12:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 9/28/21 12:11 PM, Arnd Bergmann wrote:
> > It's already upstream, see d5f6545934c4 ("qnx4: work around gcc
> > false positive warning bug").
>
> Ah, actually you mean: 9b3b353ef330 ("vboxfs: fix broken legacy mount
> signature checking"),

Yes, that's the right one of course.

      Arnd

Re: [PATCH] vboxsf: fix old signature detection

[Rasmus Villemoes]

On 27/09/2021 15.02, Dan Carpenter wrote:
> GCC handles it the same way as Clang.  '\377' is -1 but in Sparse it's
> 255.  I've added the Sparse mailing list to the CC.

FTR, while examples are not normative, this:

EXAMPLE 2 Consider implementations that use two's complement
representation for integers and eight bits for objects that have type
char. In an implementation in which type char has the same range of
values as signed char, the integer character constant '\xFF' has the
value -1; if type char has the same range of values as unsigned char,
the character constant '\xFF' has the value +255.

doesn't leave any ambiguity or (implementation|un)-definednes, and
sparse interpreting '\377' as 255 independent of its
target->unsigned_char is a plain bug in sparse.

Rasmus