[PATCH v6 4/4] Netlink: handle multi netlink messages

[Fabien Siron]

Quoting Dmitry V. Levin (2016-06-21 17:43:49)
> 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.

It seems that the check has to be moved at the beginning of the function, indeed.

> 
> > +             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?

The following fix should be okay:

return addr + NLMSG_ALIGN(nlmsghdr->nlmsg_len) > addr
       ? addr + NLMSG_ALIGN(nlmsghdr->nlmsg_len) : 0;
       
> 
> > +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)?

I forgot to replace the first condition by:
  	 if (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?
> 

If nlmsghdr.nlmsg_len is equal to -1, it means that umoven had failed.
If nlmsghdr.nlmsg_len is equal to 0, it means that nlmsg_fetch had failed.
I am not 100% sure of the last test but it seems that it's a correct way to
detect if there were several messages. 

Have you got another idea to do this test?

Regards,
--
Fabien Siron