[PATCH v8 3/4] Netlink: handle multi netlink messages

[Fabien Siron]

Quoting Dmitry V. Levin (2016-07-04 22:14:40)
> On Wed, Jun 29, 2016 at 12:20:00PM +0000, Fabien Siron wrote:
> > +static unsigned long
> > +next_nlmsg(struct nlmsghdr *nlmsghdr, unsigned long addr, unsigned long *len) {
> > +     if (NLMSG_ALIGN(nlmsghdr->nlmsg_len) == 0 ||
> > +         NLMSG_ALIGN(nlmsghdr->nlmsg_len) > *len) {
> > +             *len = NLMSG_ALIGN(nlmsghdr->nlmsg_len);
> > +             return 0;
> > +     }
> 
> *len may increase as result
> 
> > +     *len -= NLMSG_ALIGN(nlmsghdr->nlmsg_len);
> > +
> > +     if (addr + NLMSG_ALIGN(nlmsghdr->nlmsg_len) > addr) {
> > +             return addr + NLMSG_ALIGN(nlmsghdr->nlmsg_len);
> > +     } else {
> > +             *len = 0;
> > +             return 0;
> > +     }
> > +}
> 
> Could it be written in a more clear and robust way?  For example,
> 
> unsigned long nlmsg_len = NLMSG_ALIGN(nlmsghdr->nlmsg_len);
> 
> if (nlmsg_len < sizeof(struct nlmsghdr)) {
>         addr = 0;
>         nlmsg_len = sizeof(struct nlmsghdr);
> }
> 
> if (*len >= nlmsg_len)
>         *len -= nlmsg_len;
> else
>         *len = 0;
> 
> if (addr && addr + nlmsg_len > addr)
>         return addr + nlmsg_len;
> else
>         return 0;

Done.

> > +static void
> > +decode_netlink_msg(struct tcb *tcp, struct nlmsghdr *nlmsghdr,
> > +                unsigned long addr, unsigned long size)
> > +{
> >       if (size < sizeof(struct nlmsghdr)) {
> >               printstr(tcp, addr, size);
> >               return;
> >       }
> > -     if (umove_or_printaddr(tcp, addr, &nlmsghdr))
> > -             return;
> >  
> > -     tprintf("{{len=%u, type=", nlmsghdr.nlmsg_len);
> > +     tprintf("{{len=%u, type=", nlmsghdr->nlmsg_len);
> >  
> > -     printxval(netlink_types, nlmsghdr.nlmsg_type, "NLMSG_???");
> > +     printxval(netlink_types, nlmsghdr->nlmsg_type, "NLMSG_???");
> >  
> >       tprints(", flags=");
> > -     printflags(netlink_flags, nlmsghdr.nlmsg_flags, "NLM_F_???");
> > +     printflags(netlink_flags, nlmsghdr->nlmsg_flags, "NLM_F_???");
> >       /* manage get/new requests */
> >  
> > -     tprintf(", seq=%u, pid=%u}", nlmsghdr.nlmsg_seq,
> > -             nlmsghdr.nlmsg_pid);
> > +     tprintf(", seq=%u, pid=%u}", nlmsghdr->nlmsg_seq,
> > +             nlmsghdr->nlmsg_pid);
> >  
> > -     if (size - sizeof(struct nlmsghdr) > 0) {
> > +     if (nlmsghdr->nlmsg_len - sizeof(struct nlmsghdr) > 0) {
> >               tprints(", ");
> > +
> >               printstr(tcp, addr + sizeof(struct nlmsghdr),
> > -                      size - sizeof(struct nlmsghdr));
> > +                      nlmsghdr->nlmsg_len - sizeof(struct nlmsghdr));
> >       }
> >  
> >       tprints("}");
> >  }
> > +
> > +void
> > +decode_netlink(struct tcb *tcp, unsigned long addr, unsigned long total_size) {
> > +     struct nlmsghdr nlmsghdr;
> > +     unsigned long elt, size = total_size;
> > +     int print_array = 0;
> > +
> > +     for (elt = 0; fetch_nlmsg(tcp, &nlmsghdr, addr, size);
> > +          addr = next_nlmsg(&nlmsghdr, addr, &size), elt++) {
> > +             if (elt == max_strlen && abbrev(tcp)) {
> > +                     tprints("...");
> > +                     break;
> > +             }
> > +             if (nlmsghdr.nlmsg_len < sizeof(struct nlmsghdr))
> > +                     break;
> 
> This edition of decode_netlink will print nothing if addr == NULL,
> or if the first nlmsghdr.nlmsg_len < sizeof(struct nlmsghdr).

What should be printed in these cases?

> 
> Please add to the test all pathological cases we discussed so far.
> 

So let's add a test where nlmsghdr->nlmsg_len is 0 and a test with
nlmsghdr->nlmsg_len is greater than len. Do you have other pathological cases
in mind?

Cheers,
--
Fabien Siron