linux-kernel - RE: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BYAPR21MB1688422E9CD742B482248E8BD7D99@BYAPR21MB1688.namprd21.prod.outlook.com>
Date:   Thu, 9 Feb 2023 17:10:56 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     Haiyang Zhang <haiyangz@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT
 completion message

From: Haiyang Zhang <haiyangz@...rosoft.com> Sent: Thursday, February 9, 2023 5:49 AM
> 
> > -----Original Message-----
> > From: Michael Kelley (LINUX) <mikelley@...rosoft.com>
> > Sent: Wednesday, February 8, 2023 6:50 PM
> > To: KY Srinivasan <kys@...rosoft.com>; Haiyang Zhang
> > <haiyangz@...rosoft.com>; wei.liu@...nel.org; Dexuan Cui
> > <decui@...rosoft.com>; davem@...emloft.net; edumazet@...gle.com;
> > kuba@...nel.org; pabeni@...hat.com; netdev@...r.kernel.org; linux-
> > hyperv@...r.kernel.org; linux-kernel@...r.kernel.org
> > Cc: Michael Kelley (LINUX) <mikelley@...rosoft.com>
> > Subject: [PATCH net-next 1/1] hv_netvsc: Check status in SEND_RNDIS_PKT
> > completion message
> >
> > Completion responses to SEND_RNDIS_PKT messages are currently processed
> > regardless of the status in the response, so that resources associated
> > with the request are freed.  While this is appropriate, code bugs that
> > cause sending a malformed message, or errors on the Hyper-V host, go
> > undetected. Fix this by checking the status and outputting a message
> > if there is an error.
> >
> > Signed-off-by: Michael Kelley <mikelley@...rosoft.com>
> > ---
> >  drivers/net/hyperv/netvsc.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> > index 661bbe6..caf22e9 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -813,6 +813,7 @@ static void netvsc_send_completion(struct net_device *ndev,
> >  	u32 msglen = hv_pkt_datalen(desc);
> >  	struct nvsp_message *pkt_rqst;
> >  	u64 cmd_rqst;
> > +	u32 status;
> >
> >  	/* First check if this is a VMBUS completion without data payload */
> >  	if (!msglen) {
> > @@ -884,6 +885,22 @@ static void netvsc_send_completion(struct net_device *ndev,
> >  		break;
> >
> >  	case NVSP_MSG1_TYPE_SEND_RNDIS_PKT_COMPLETE:
> > +		if (msglen < sizeof(struct nvsp_message_header) +
> > +		    sizeof(struct nvsp_1_message_send_rndis_packet_complete)) {
> > +			netdev_err(ndev, "nvsp_rndis_pkt_complete length too small: %u\n",
> > +				   msglen);
> > +			return;
> > +		}
> > +
> > +		/* If status indicates an error, output a message so we know
> > +		 * there's a problem. But process the completion anyway so the
> > +		 * resources are released.
> > +		 */
> > +		status = nvsp_packet->msg.v1_msg.send_rndis_pkt_complete.status;
> > +		if (status != NVSP_STAT_SUCCESS)
> > +			netdev_err(ndev, "nvsp_rndis_pkt_complete error status: %x\n",
> > +				   status);
> > +
> 
> Could you add rate limit to this error, so in case it happens frequently, the
> errors won't fill up the dmesg.
> 
> Or even better, add a counter for this.

I thought about rate limiting.  But my assumption is that such errors are
very rare, and that it would be better to see all occurrences instead of
potentially filtering some out due to rate limiting.  If that assumption
proves to not be true, then we probably have a bigger problem -- there's
a bug in the Linux guest causing it to submit bad requests, or there's a
bug on the Hyper-V side.

That said, I don't feel strongly about it either way. 

Thoughts?

Michael