diff mbox

[v2,18/19] linux-user: Avoid possible misalignment in host_to_target_siginfo()

Message ID 1464360721-14359-19-git-send-email-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Maydell May 27, 2016, 2:52 p.m. UTC
host_to_target_siginfo() is implemented by a combination of
host_to_target_siginfo_noswap() followed by tswap_siginfo().
The first of these two functions assumes that the target_siginfo_t
it is writing to is correctly aligned, but the pointer passed
into host_to_target_siginfo() is directly from the guest and
might be misaligned. Use a local variable to avoid this problem.
(tswap_siginfo() does now correctly handle a misaligned destination.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Laurent Vivier June 7, 2016, 7:36 p.m. UTC | #1
Le 27/05/2016 à 16:52, Peter Maydell a écrit :
> host_to_target_siginfo() is implemented by a combination of
> host_to_target_siginfo_noswap() followed by tswap_siginfo().
> The first of these two functions assumes that the target_siginfo_t
> it is writing to is correctly aligned, but the pointer passed
> into host_to_target_siginfo() is directly from the guest and
> might be misaligned. Use a local variable to avoid this problem.
> (tswap_siginfo() does now correctly handle a misaligned destination.)

You mean the pointer from the guest can not be correctly aligned for the
guest?

Laurent
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/signal.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 8ea0cbf..7e2a80f 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -400,8 +400,9 @@ static void tswap_siginfo(target_siginfo_t *tinfo,
>  
>  void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info)
>  {
> -    host_to_target_siginfo_noswap(tinfo, info);
> -    tswap_siginfo(tinfo, tinfo);
> +    target_siginfo_t tgt_tmp;
> +    host_to_target_siginfo_noswap(&tgt_tmp, info);
> +    tswap_siginfo(tinfo, &tgt_tmp);
>  }
>  
>  /* XXX: we support only POSIX RT signals are used. */
>
Peter Maydell June 7, 2016, 9:08 p.m. UTC | #2
On 7 June 2016 at 20:36, Laurent Vivier <laurent@vivier.eu> wrote:
>
>
> Le 27/05/2016 à 16:52, Peter Maydell a écrit :
>> host_to_target_siginfo() is implemented by a combination of
>> host_to_target_siginfo_noswap() followed by tswap_siginfo().
>> The first of these two functions assumes that the target_siginfo_t
>> it is writing to is correctly aligned, but the pointer passed
>> into host_to_target_siginfo() is directly from the guest and
>> might be misaligned. Use a local variable to avoid this problem.
>> (tswap_siginfo() does now correctly handle a misaligned destination.)
>
> You mean the pointer from the guest can not be correctly aligned for the
> guest?

Might not be correctly aligned for the host (for that matter
it might not be correctly aligned for the guest,
if the guest is being malicious or buggy, but it's the
host alignment we care about.)

thanks
-- PMM
Laurent Vivier June 8, 2016, 9:29 a.m. UTC | #3
Le 07/06/2016 à 23:08, Peter Maydell a écrit :
> On 7 June 2016 at 20:36, Laurent Vivier <laurent@vivier.eu> wrote:
>>
>>
>> Le 27/05/2016 à 16:52, Peter Maydell a écrit :
>>> host_to_target_siginfo() is implemented by a combination of
>>> host_to_target_siginfo_noswap() followed by tswap_siginfo().
>>> The first of these two functions assumes that the target_siginfo_t
>>> it is writing to is correctly aligned, but the pointer passed
>>> into host_to_target_siginfo() is directly from the guest and
>>> might be misaligned. Use a local variable to avoid this problem.
>>> (tswap_siginfo() does now correctly handle a misaligned destination.)
>>
>> You mean the pointer from the guest can not be correctly aligned for the
>> guest?
> 
> Might not be correctly aligned for the host (for that matter
> it might not be correctly aligned for the guest,
> if the guest is being malicious or buggy, but it's the
> host alignment we care about.)

Because of the "abi_ulong _addr", I think this structure is always
aligned for the guest.

Laurent
Peter Maydell June 8, 2016, 11:29 a.m. UTC | #4
On 8 June 2016 at 10:29, Laurent Vivier <laurent@vivier.eu> wrote:
>
>
> Le 07/06/2016 à 23:08, Peter Maydell a écrit :
>> On 7 June 2016 at 20:36, Laurent Vivier <laurent@vivier.eu> wrote:
>>>
>>>
>>> Le 27/05/2016 à 16:52, Peter Maydell a écrit :
>>>> host_to_target_siginfo() is implemented by a combination of
>>>> host_to_target_siginfo_noswap() followed by tswap_siginfo().
>>>> The first of these two functions assumes that the target_siginfo_t
>>>> it is writing to is correctly aligned, but the pointer passed
>>>> into host_to_target_siginfo() is directly from the guest and
>>>> might be misaligned. Use a local variable to avoid this problem.
>>>> (tswap_siginfo() does now correctly handle a misaligned destination.)
>>>
>>> You mean the pointer from the guest can not be correctly aligned for the
>>> guest?
>>
>> Might not be correctly aligned for the host (for that matter
>> it might not be correctly aligned for the guest,
>> if the guest is being malicious or buggy, but it's the
>> host alignment we care about.)
>
> Because of the "abi_ulong _addr", I think this structure is always
> aligned for the guest.

No, because the address of the structure in guest memory comes from
a pointer passed to us by the guest. The guest could pass any value
at all that it likes for that pointer including one that is arbitrarily
misaligned. I think it's pretty much always a bug to do a direct
access to a structure field that's in guest memory, though we do
it a fair bit and get away with it because x86-64 doesn't have
alignment restrictions.

thanks
-- PMM
diff mbox

Patch

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 8ea0cbf..7e2a80f 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -400,8 +400,9 @@  static void tswap_siginfo(target_siginfo_t *tinfo,
 
 void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info)
 {
-    host_to_target_siginfo_noswap(tinfo, info);
-    tswap_siginfo(tinfo, tinfo);
+    target_siginfo_t tgt_tmp;
+    host_to_target_siginfo_noswap(&tgt_tmp, info);
+    tswap_siginfo(tinfo, &tgt_tmp);
 }
 
 /* XXX: we support only POSIX RT signals are used. */