Re: [PATCH v2] linux-user: implement TARGET_SO_PEERSEC

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/12/20 5:03 PM, Laurent Vivier wrote:
> Le 12/02/2020 à 16:56, Philippe Mathieu-Daudé a écrit :
>> On 2/4/20 10:19 PM, Laurent Vivier wrote:
>>> "The purpose of this option is to allow an application to obtain the
>>> security credentials of a Unix stream socket peer.  It is analogous to
>>> SO_PEERCRED (which provides authentication using standard Unix
>>> credentials
>>> of pid, uid and gid), and extends this concept to other security
>>> models." -- https://lwn.net/Articles/62370/
>>>
>>> Until now it was passed to the kernel with an "int" argument and
>>> fails when it was supported by the host because the parameter is
>>> like a filename: it is always a \0-terminated string with no embedded
>>> \0 characters, but is not guaranteed to be ASCII or UTF-8.
>>>
>>> I've tested the option with the following program:
>>>
>>>       /*
>>>        * cc -o getpeercon getpeercon.c
>>>        */
>>>
>>>       #include <stdio.h>
>>>       #include <sys/types.h>
>>>       #include <sys/socket.h>
>>>       #include <netinet/in.h>
>>>       #include <arpa/inet.h>
>>>
>>>       int main(void)
>>>       {
>>>           int fd;
>>>           struct sockaddr_in server, addr;
>>>           int ret;
>>>           socklen_t len;
>>>           char buf[256];
>>>
>>>           fd = socket(PF_INET, SOCK_STREAM, 0);
>>>           if (fd == -1) {
>>>               perror("socket");
>>>               return 1;
>>>           }
>>>
>>>           server.sin_family = AF_INET;
>>>           inet_aton("127.0.0.1", &server.sin_addr);
>>>           server.sin_port = htons(40390);
>>>
>>>           connect(fd, (struct sockaddr*)&server, sizeof(server));
>>>
>>>           len = sizeof(buf);
>>>           ret = getsockopt(fd, SOL_SOCKET, SO_PEERSEC, buf, &len);
>>>           if (ret == -1) {
>>>               perror("getsockopt");
>>>               return 1;
>>>           }
>>>           printf("%d %s\n", len, buf);
>>>           return 0;
>>>       }
>>>
>>> On host:
>>>
>>>     $ ./getpeercon
>>>     33 system_u:object_r:unlabeled_t:s0
>>>
>>> With qemu-aarch64/bionic without the patch:
>>>
>>>     $ ./getpeercon
>>>     getsockopt: Numerical result out of range
>>>
>>> With the patch:
>>>
>>>     $ ./getpeercon
>>>     33 system_u:object_r:unlabeled_t:s0
>>>
>>> Bug: https://bugs.launchpad.net/qemu/+bug/1823790
>>> Reported-by: Matthias Lüscher <lueschem@gmail.com>
>>> Tested-by: Matthias Lüscher <lueschem@gmail.com>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>>
>>> Notes:
>>>       v2: use correct length in unlock_user()
>>>
>>>    linux-user/syscall.c | 22 ++++++++++++++++++++++
>>>    1 file changed, 22 insertions(+)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index d60142f0691c..c930577686da 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -2344,6 +2344,28 @@ static abi_long do_getsockopt(int sockfd, int
>>> level, int optname,
>>>                }
>>>                break;
>>>            }
>>> +        case TARGET_SO_PEERSEC: {
>>> +            char *name;
>>> +
>>> +            if (get_user_u32(len, optlen)) {
>>> +                return -TARGET_EFAULT;
>>> +            }
>>> +            if (len < 0) {
>>> +                return -TARGET_EINVAL;
>>> +            }
>>> +            name = lock_user(VERIFY_WRITE, optval_addr, len, 0);
>>> +            if (!name) {
>>> +                return -TARGET_EFAULT;
>>> +            }
>>> +            lv = len;
>>> +            ret = get_errno(getsockopt(sockfd, level, SO_PEERSEC,
>>> +                                       name, &lv));
>>
>> Can we get lv > len?
> 
> No:
> 
> getsockopt(2)
> 
> "For  getsockopt(), optlen is a value-result argument, initially
> containing the size of the buffer pointed to by optval, and modified on
> return to  indicate the  actual  size  of  the value returned."
> 
>>
>>> +            if (put_user_u32(lv, optlen)) {
>>> +                ret = -TARGET_EFAULT;
>>> +            }
>>> +            unlock_user(name, optval_addr, lv);
>>
>> Maybe safer to use len instead of lv here?
> 
> No:
> 
> this is the length of the buffer we must copy back to the user. Kernel
> has only modified lv length, not len.

So we can simplify the TARGET_SO_LINGER case then.

> 
> linux-user/qemu.h
> 
> /* Unlock an area of guest memory.  The first LEN bytes must be
>     flushed back to guest memory. host_ptr = NULL is explicitly
>     allowed and does nothing. */
> static inline void unlock_user(void *host_ptr, abi_ulong guest_addr,
>                                 long len)
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>