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