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

[Dmitry V. Levin]

On Wed, Jun 22, 2016 at 10:19:03AM +0000, Fabien Siron wrote:
> 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;

Remember to set *len to 0 in case of overflow,
otherwise the subsequent nlmsg_fetch will print NULL.
> > 
> > > +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))

Only if nlmsghdr.nlmsg_len has been validated.

Why decode_netlink_msg fetches the same nlmsghdr right after it's just
been fetched and validated by nlmsg_fetch?

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

No.

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

Just set a flag if "[" has been printed, and test it afterwards.

Also, nlmsg_fetch returns 0 when nlmsghdr->nlmsg_len doesn't look sane.
Shouldn't strace print something meaningful in tis case, rather than
silently ignore invalid nlmsghdr?


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20160622/909d4372/attachment.bin>