path: root/classification_output/03/other/02364653

other: 0.956
semantic: 0.942
instruction: 0.927
boot: 0.925
mistranslation: 0.912
KVM: 0.911
network: 0.881

[Qemu-devel] [BUG] Inappropriate size of target_sigset_t

Hello, Peter, Laurent,

While working on another problem yesterday, I think I discovered a 
long-standing bug in QEMU Linux user mode: our target_sigset_t structure is 
eight times smaller as it should be!

In this code segment from syscalls_def.h:

#ifdef TARGET_MIPS
#define TARGET_NSIG        128
#else
#define TARGET_NSIG        64
#endif
#define TARGET_NSIG_BPW    TARGET_ABI_BITS
#define TARGET_NSIG_WORDS  (TARGET_NSIG / TARGET_NSIG_BPW)

typedef struct {
    abi_ulong sig[TARGET_NSIG_WORDS];
} target_sigset_t;

... TARGET_ABI_BITS should be replaced by eight times smaller constant (in 
fact, semantically, we need TARGET_ABI_BYTES, but it is not defined) (what is 
needed is actually "a byte per signal" in target_sigset_t, and we allow "a bit 
per signal").

All this probably sounds to you like something impossible, since this code is 
in QEMU "since forever", but I checked everything, and the bug seems real. I 
wish you can prove me wrong.

I just wanted to let you know about this, given the sensitive timing of current 
softfreeze, and the fact that I won't be able to do more investigation on this 
in coming weeks, since I am busy with other tasks, but perhaps you can analyze 
and do something which you consider appropriate.

Yours,
Aleksandar

Le 03/07/2019 à 21:46, Aleksandar Markovic a écrit :
>
Hello, Peter, Laurent,
>
>
While working on another problem yesterday, I think I discovered a
>
long-standing bug in QEMU Linux user mode: our target_sigset_t structure is
>
eight times smaller as it should be!
>
>
In this code segment from syscalls_def.h:
>
>
#ifdef TARGET_MIPS
>
#define TARGET_NSIG      128
>
#else
>
#define TARGET_NSIG      64
>
#endif
>
#define TARGET_NSIG_BPW          TARGET_ABI_BITS
>
#define TARGET_NSIG_WORDS  (TARGET_NSIG / TARGET_NSIG_BPW)
>
>
typedef struct {
>
abi_ulong sig[TARGET_NSIG_WORDS];
>
} target_sigset_t;
>
>
... TARGET_ABI_BITS should be replaced by eight times smaller constant (in
>
fact, semantically, we need TARGET_ABI_BYTES, but it is not defined) (what is
>
needed is actually "a byte per signal" in target_sigset_t, and we allow "a
>
bit per signal").
TARGET_NSIG is divided by TARGET_ABI_BITS which gives you the number of
abi_ulong words we need in target_sigset_t.

>
All this probably sounds to you like something impossible, since this code is
>
in QEMU "since forever", but I checked everything, and the bug seems real. I
>
wish you can prove me wrong.
>
>
I just wanted to let you know about this, given the sensitive timing of
>
current softfreeze, and the fact that I won't be able to do more
>
investigation on this in coming weeks, since I am busy with other tasks, but
>
perhaps you can analyze and do something which you consider appropriate.
If I compare with kernel, it looks good:

In Linux:

  arch/mips/include/uapi/asm/signal.h

  #define _NSIG           128
  #define _NSIG_BPW       (sizeof(unsigned long) * 8)
  #define _NSIG_WORDS     (_NSIG / _NSIG_BPW)

  typedef struct {
          unsigned long sig[_NSIG_WORDS];
  } sigset_t;

_NSIG_BPW is 8 * 8 = 64 on MIPS64 or 4 * 8 = 32 on MIPS

In QEMU:

TARGET_NSIG_BPW is TARGET_ABI_BITS which is  TARGET_LONG_BITS which is
64 on MIPS64 and 32 on MIPS.

I think there is no problem.

Thanks,
Laurent

From: Laurent Vivier <address@hidden>
>
If I compare with kernel, it looks good:
>
...
>
I think there is no problem.
Sure, thanks for such fast response - again, I am glad if you are right. 
However, for some reason, glibc (and musl too) define sigset_t differently than 
kernel. Please take a look. I am not sure if this is covered fine in our code.

Yours,
Aleksandar

>
Thanks,
>
Laurent

On Wed, 3 Jul 2019 at 21:20, Aleksandar Markovic <address@hidden> wrote:
>
>
From: Laurent Vivier <address@hidden>
>
> If I compare with kernel, it looks good:
>
> ...
>
> I think there is no problem.
>
>
Sure, thanks for such fast response - again, I am glad if you are right.
>
However, for some reason, glibc (and musl too) define sigset_t differently
>
than kernel. Please take a look. I am not sure if this is covered fine in our
>
code.
Yeah, the libc definitions of sigset_t don't match the
kernel ones (this is for obscure historical reasons IIRC).
We're providing implementations of the target
syscall interface, so our target_sigset_t should be the
target kernel's version (and the target libc's version doesn't
matter to us). On the other hand we will be using the
host libc version, I think, so a little caution is required
and it's possible we have some bugs in our code.

thanks
-- PMM

>
From: Peter Maydell <address@hidden>
>
>
On Wed, 3 Jul 2019 at 21:20, Aleksandar Markovic <address@hidden> wrote:
>
>
>
> From: Laurent Vivier <address@hidden>
>
> > If I compare with kernel, it looks good:
>
> > ...
>
> > I think there is no problem.
>
>
>
> Sure, thanks for such fast response - again, I am glad if you are right.
>
> However, for some reason, glibc (and musl too) define sigset_t differently
>
> than kernel. Please take a look. I am not sure if this is covered fine in
>
> our code.
>
>
Yeah, the libc definitions of sigset_t don't match the
>
kernel ones (this is for obscure historical reasons IIRC).
>
We're providing implementations of the target
>
syscall interface, so our target_sigset_t should be the
>
target kernel's version (and the target libc's version doesn't
>
matter to us). On the other hand we will be using the
>
host libc version, I think, so a little caution is required
>
and it's possible we have some bugs in our code.
OK, I gather than this is not something that requires our immediate attention 
(for 4.1), but we can analyze it later on.

Thanks for response!!

Sincerely,
Aleksandar

>
thanks
>
-- PMM

Le 03/07/2019 à 22:28, Peter Maydell a écrit :
>
On Wed, 3 Jul 2019 at 21:20, Aleksandar Markovic <address@hidden> wrote:
>
>
>
> From: Laurent Vivier <address@hidden>
>
>> If I compare with kernel, it looks good:
>
>> ...
>
>> I think there is no problem.
>
>
>
> Sure, thanks for such fast response - again, I am glad if you are right.
>
> However, for some reason, glibc (and musl too) define sigset_t differently
>
> than kernel. Please take a look. I am not sure if this is covered fine in
>
> our code.
>
>
Yeah, the libc definitions of sigset_t don't match the
>
kernel ones (this is for obscure historical reasons IIRC).
>
We're providing implementations of the target
>
syscall interface, so our target_sigset_t should be the
>
target kernel's version (and the target libc's version doesn't
>
matter to us). On the other hand we will be using the
>
host libc version, I think, so a little caution is required
>
and it's possible we have some bugs in our code.
It's why we need host_to_target_sigset_internal() and
target_to_host_sigset_internal() that translates bits and bytes between
guest kernel interface and host libc interface.

void host_to_target_sigset_internal(target_sigset_t *d,
                                    const sigset_t *s)
{
    int i;
    target_sigemptyset(d);
    for (i = 1; i <= TARGET_NSIG; i++) {
        if (sigismember(s, i)) {
            target_sigaddset(d, host_to_target_signal(i));
        }
    }
}

void target_to_host_sigset_internal(sigset_t *d,
                                    const target_sigset_t *s)
{
    int i;
    sigemptyset(d);
    for (i = 1; i <= TARGET_NSIG; i++) {
        if (target_sigismember(s, i)) {
            sigaddset(d, target_to_host_signal(i));
        }
    }
}

Thanks,
Laurent

Hi Aleksandar,

On Wed, Jul 3, 2019 at 12:48 PM Aleksandar Markovic
<address@hidden> wrote:
>
#define TARGET_NSIG_BPW    TARGET_ABI_BITS
>
#define TARGET_NSIG_WORDS  (TARGET_NSIG / TARGET_NSIG_BPW)
>
>
typedef struct {
>
abi_ulong sig[TARGET_NSIG_WORDS];
>
} target_sigset_t;
>
>
... TARGET_ABI_BITS should be replaced by eight times smaller constant (in
>
fact,
>
semantically, we need TARGET_ABI_BYTES, but it is not defined) (what is needed
>
is actually "a byte per signal" in target_sigset_t, and we allow "a bit per
>
signal").
Why do we need a byte per target signal, if the functions in linux-user/signal.c
operate with bits?

-- 
Thanks.
-- Max

>
Why do we need a byte per target signal, if the functions in
>
linux-user/signal.c
>
operate with bits?
Max,

I did not base my findings on code analysis, but on dumping size/offsets of 
elements of some structures, as they are emulated in QEMU, and in real systems. 
So, I can't really answer your question.

Yours,
Aleksandar

>
--
>
Thanks.
>
-- Max