Re: [PATCH 1/2] linux-user: add do_setsockopt SOL_CAN_RAW CAN_RAW_FILTER support

[Laurent Vivier]

Le 06/05/2020 à 15:21, Tomas Krcka a écrit :
> Signed-off-by: Tomas Krcka <address@hidden>
> ---
>  linux-user/syscall.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 05f03919ff..88d4c85b70 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -56,6 +56,7 @@
>  #include <linux/wireless.h>
>  #include <linux/icmp.h>
>  #include <linux/icmpv6.h>
> +#include <linux/can/raw.h>
>  #include <linux/errqueue.h>
>  #include <linux/random.h>
>  #ifdef CONFIG_TIMERFD
> @@ -2111,6 +2112,39 @@ static abi_long do_setsockopt(int sockfd, int level, 
> int optname,
>              goto unimplemented;
>          }
>          break;
> +    case SOL_CAN_RAW:
> +        switch (optname) {
> +        case CAN_RAW_FILTER:
> +        {
> +            if (optlen % sizeof(struct can_filter) != 0) {
> +                return -TARGET_EINVAL;
> +            }
> +
> +            struct can_filter  *can_filters = NULL;

Move the declaration to the top of the block.

> +            if (optlen != 0) {

If you check, like in kernel, "optlen > CAN_RAW_FILTER_MAX *
sizeof(struct can_filter)", you can exit here (and no need to set
can_filters to NULL).

> +                can_filters = g_new0(struct can_filter, optlen);
> +
> +                if (!can_filters) {

no need to check the result, g_new0() aborts in case of problem.

> +                    return -TARGET_ENOMEM;
> +                }
> +                if (copy_from_user(can_filters, optval_addr, optlen)) {
> +                    g_free(can_filters);
> +                    return -TARGET_EFAULT;
> +                }

It would be cleaner not to use the copy_from_user() as we need to
byte-swap all the fields in the loop below (I know it's done like that
in SOL_ICMPV6...)

> +                for (val = 0; val < optlen / sizeof(struct can_filter); 
> val++) {
> +                    can_filters[val].can_id = 
> tswap32(can_filters[val].can_id);
> +                    can_filters[val].can_mask = 
> tswap32(can_filters[val].can_mask);
> +                }

So, something like:

target_can_filters = lock_user(VERIFY_READ, optval_addr, optlen, 1);
for (val = 0; val < optlen / sizeof(struct can_filter); val++) {
    __get_user(can_filters[val].can_id, \
               &target_can_filters[val].can_id);
    __get_user(can_filters[val].can_mask, \
               &target_can_filters[val].can_mask);
}
unlock_user(target_can_filters);

> +            }
> +            ret = get_errno(setsockopt(sockfd, level, optname,
> +                                        can_filters, optlen));
> +            g_free(can_filters);
> +            break;
> +        }
> +        default:
> +            goto unimplemented;
> +        }
> +        break;
>      case SOL_RAW:
>          switch (optname) {
>          case ICMP_FILTER:
> 

Could you also update getsockopt()?

Thanks,
Laurent