On Tue, Jun 21, 2016 at 02:42:46PM +0000, Fabien Siron wrote:
[...]
> +static unsigned long
> +nlmsg_next(struct nlmsghdr *nlmsghdr, unsigned long addr, unsigned long 
> *len) {
> +     *len -= NLMSG_ALIGN(nlmsghdr->nlmsg_len);
> +
> +     if (NLMSG_ALIGN(nlmsghdr->nlmsg_len) == 0 ||
> +         NLMSG_ALIGN(nlmsghdr->nlmsg_len) > *len)

The check is too late.

> +             return 0;
> +
> +     return (unsigned long)((addr) + NLMSG_ALIGN(nlmsghdr->nlmsg_len));
> +}

Why (addr)?  Is this cast to (unsigned long) really needed?
What's going to happen in case of integer overflow?

> +static void
> +decode_netlink_msg(struct tcb *tcp, unsigned long addr,
> +                         unsigned long size)
>  {
>       struct nlmsghdr nlmsghdr;
>  
> @@ -57,8 +86,32 @@ decode_netlink(struct tcb *tcp, unsigned long addr, 
> unsigned long size)
>       if (size - sizeof(struct nlmsghdr) > 0) {
>               tprints(", ");
>               printstr(tcp, addr + sizeof(struct nlmsghdr),
> -                      size - sizeof(struct nlmsghdr));
> +                      nlmsghdr.nlmsg_len - sizeof(struct nlmsghdr));
>       }
>       tprints("}");
>  }

What's going to be printed if size > sizeof(struct nlmsghdr)
AND nlmsghdr.nlmsg_len == sizeof(struct nlmsghdr)?

> +
> +void
> +decode_netlink(struct tcb *tcp, unsigned long addr, unsigned long 
> total_size) {
> +     struct nlmsghdr nlmsghdr;
> +     unsigned long elt, size = total_size;
> +
> +     for (elt = 0; nlmsg_fetch(tcp, &nlmsghdr, addr, size);
> +          addr = nlmsg_next(&nlmsghdr, addr, &size), elt++) {
> +             if (elt == max_strlen && abbrev(tcp)) {
> +                     tprints("...");
> +                     break;
> +             }
> +             if (size != total_size)
> +                     tprints(", ");
> +             else if (NLMSG_ALIGN(nlmsghdr.nlmsg_len) < total_size)
> +                     tprints("[");
> +             decode_netlink_msg(tcp, addr, size);
> +     }
> +     if (nlmsghdr.nlmsg_len != (unsigned) -1 &&
> +         nlmsghdr.nlmsg_len != 0 &&
> +         NLMSG_ALIGN(nlmsghdr.nlmsg_len) < total_size) {
> +             tprints("]");
> +     }
> +}

This check doesn't look obvious; is it correct?
Is there a more clear way to implement this?


-- 
ldv

Attachment: pgpG_6upRXrfI.pgp
Description: PGP signature

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel

Reply via email to