+ Ricardo, Eric, Jakub, and Paolo Please derive CC list from get_maintainers.pl my.patch On Fri, Jul 05, 2024 at 07:08:08PM +0300, Dmitry Antipov wrote: > Since 'ppp_async_encode()' assumes valid LCP packets (with code > from 1 to 7 inclusive), add 'ppp_check_packet()' to ensure that > LCP packet has an actual body beyond PPP_LCP header bytes, and > reject claimed-as-LCP but actually malformed data otherwise. > > Reported-by: syzbot+ec0723ba9605678b14bf@xxxxxxxxxxxxxxxxxxxxxxxxx > Closes: https://syzkaller.appspot.com/bug?extid=ec0723ba9605678b14bf Hi Dmitry, As a fix, a Fixes tag should go here (no blank line between any tags). And the patch should be targeted at the net tree: Subject: [PATCH net] ... > Signed-off-by: Dmitry Antipov <dmantipov@xxxxxxxxx> > --- > drivers/net/ppp/ppp_generic.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c > index 0a65b6d690fe..2c8dfeb8ca58 100644 > --- a/drivers/net/ppp/ppp_generic.c > +++ b/drivers/net/ppp/ppp_generic.c > @@ -493,6 +493,18 @@ static ssize_t ppp_read(struct file *file, char __user *buf, > return ret; > } > > +static bool ppp_check_packet(struct sk_buff *skb, size_t count) > +{ > + if (get_unaligned_be16(skb->data) == PPP_LCP && > + count < PPP_PROTO_LEN + 4) > + /* Claimed as LCP but has no actual LCP body, > + * which is 4 bytes at least (code, identifier, > + * and 2-byte length). > + */ > + return false; > + return true; > +} I agree that this fix is correct, that it addresses the issue at hand, and that ppp_write() is a good place for this check for invalid input. But I have some minor feedback on the implementation above. 1. It might be nicer to add define, say near where PPP_PROTO_LEN is defined, instead of using 4. E.g. #define PPP_LCP_HDR_LEN 4 2. I would express the boolean logic without an if condition: (Completely untested!) static bool ppp_check_packet(struct sk_buff *skb, size_t count) { /* LCP packets must include LCP header which 4 bytes long: * 1-byte code, 1-byte identifier, and 2-byte length. */ return get_unaligned_be16(skb->data) != PPP_LCP || count >= PPP_PROTO_LEN + PPP_LCP_HDR_LEN; } > + > static ssize_t ppp_write(struct file *file, const char __user *buf, > size_t count, loff_t *ppos) > { > @@ -515,6 +527,11 @@ static ssize_t ppp_write(struct file *file, const char __user *buf, > kfree_skb(skb); > goto out; > } > + ret = -EINVAL; > + if (unlikely(!ppp_check_packet(skb, count))) { > + kfree_skb(skb); > + goto out; > + } FWIIW, I agree the above is in keeping with the existing flow of this function. > > switch (pf->kind) { > case INTERFACE: > -- > 2.45.2 > >
