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

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

[Aleksandar Markovic]

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

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

[Laurent Vivier]

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

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

[Max Filippov]

Hi Aleksandar,

On Wed, Jul 3, 2019 at 12:48 PM Aleksandar Markovic
<amarkovic@wavecomp.com> 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

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

[Aleksandar Markovic]

From: Laurent Vivier <laurent@vivier.eu>
> 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

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

[Peter Maydell]

On Wed, 3 Jul 2019 at 21:20, Aleksandar Markovic <amarkovic@wavecomp.com> wrote:
>
> From: Laurent Vivier <laurent@vivier.eu>
> > 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

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

[Aleksandar Markovic]

> 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

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

[Aleksandar Markovic]

> From: Peter Maydell <peter.maydell@linaro.org>
> 
> On Wed, 3 Jul 2019 at 21:20, Aleksandar Markovic <amarkovic@wavecomp.com> wrote:
> >
> > From: Laurent Vivier <laurent@vivier.eu>
> > > 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

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

[Laurent Vivier]

Le 03/07/2019 à 22:28, Peter Maydell a écrit :
> On Wed, 3 Jul 2019 at 21:20, Aleksandar Markovic <amarkovic@wavecomp.com> wrote:
>>
>> From: Laurent Vivier <laurent@vivier.eu>
>>> 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