diff mbox series

usb: gadget: rndis: Avoid dereference before NULL check

Message ID 20220908175615.5095-1-jilin@nvidia.com (mailing list archive)
State New, archived
Headers show
Series usb: gadget: rndis: Avoid dereference before NULL check | expand

Commit Message

Jim Lin Sept. 8, 2022, 5:56 p.m. UTC 
NULL check is performed after params->dev is dereferenced in
dev_get_stats.
Fixed by adding a NULL check before dereferencing params->dev and
removing subsequent NULL checks for it.

Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
Signed-off-by: Jim Lin <jilin@nvidia.com>
---
 drivers/usb/gadget/function/rndis.c | 37 ++++++++++++-----------------
 1 file changed, 15 insertions(+), 22 deletions(-)

Comments

Sergey Shtylyov Sept. 8, 2022, 8:30 p.m. UTC | #1 
On 9/8/22 8:56 PM, Jim Lin wrote:

> NULL check is performed after params->dev is dereferenced in
> dev_get_stats.
> Fixed by adding a NULL check before dereferencing params->dev and
> removing subsequent NULL checks for it.

   You've beaten me to send such a patch... :-)

> 
> Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
> Signed-off-by: Jim Lin <jilin@nvidia.com>
> ---
>  drivers/usb/gadget/function/rndis.c | 37 ++++++++++++-----------------
>  1 file changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
> index 64de9f1b874c..d2f18f34c8e5 100644
> --- a/drivers/usb/gadget/function/rndis.c
> +++ b/drivers/usb/gadget/function/rndis.c
> @@ -198,6 +198,9 @@ static int gen_ndis_query_resp(struct rndis_params *params, u32 OID, u8 *buf,
>  	outbuf = (__le32 *)&resp[1];
>  	resp->InformationBufferOffset = cpu_to_le32(16);
>  
> +	if (!params->dev)
> +		return -ENODEV;
> +

   Hm, isn't this checked at the start of rndis_query_response(), this function's
only caller?

[...]

MBR, Sergey
Greg Kroah-Hartman Sept. 9, 2022, 5:38 a.m. UTC | #2 
On Fri, Sep 09, 2022 at 01:56:15AM +0800, Jim Lin wrote:
> NULL check is performed after params->dev is dereferenced in
> dev_get_stats.

I do not understand this statement.

> Fixed by adding a NULL check before dereferencing params->dev and
> removing subsequent NULL checks for it.
> 
> Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
> Signed-off-by: Jim Lin <jilin@nvidia.com>
> ---
>  drivers/usb/gadget/function/rndis.c | 37 ++++++++++++-----------------
>  1 file changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
> index 64de9f1b874c..d2f18f34c8e5 100644
> --- a/drivers/usb/gadget/function/rndis.c
> +++ b/drivers/usb/gadget/function/rndis.c
> @@ -198,6 +198,9 @@ static int gen_ndis_query_resp(struct rndis_params *params, u32 OID, u8 *buf,
>  	outbuf = (__le32 *)&resp[1];
>  	resp->InformationBufferOffset = cpu_to_le32(16);
>  
> +	if (!params->dev)
> +		return -ENODEV;
> +

As Sergey points out, this check is useless and the ones below should
also be removed.

But, why make this check at all, how did you trigger a problem with the
current code?

Are you using this driver?  If so, why?  It is totally broken (as per
the specification) and we really really need to just delete it from the
tree to prevent anyone else from ever using it.

thanks,

greg k-h
Jim Lin Sept. 9, 2022, 2:43 p.m. UTC | #3 
On Fri, 2022-09-09 at 07:38 +0200, Greg KH wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Fri, Sep 09, 2022 at 01:56:15AM +0800, Jim Lin wrote:
> > NULL check is performed after params->dev is dereferenced in
> > dev_get_stats.
> 
> I do not understand this statement.
> 
> > Fixed by adding a NULL check before dereferencing params->dev and
> > removing subsequent NULL checks for it.
> > 
> > Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
> > Signed-off-by: Jim Lin <jilin@nvidia.com>
> > ---
> >  drivers/usb/gadget/function/rndis.c | 37 ++++++++++++-------------
> > ----
> >  1 file changed, 15 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/rndis.c
> > b/drivers/usb/gadget/function/rndis.c
> > index 64de9f1b874c..d2f18f34c8e5 100644
> > --- a/drivers/usb/gadget/function/rndis.c
> > +++ b/drivers/usb/gadget/function/rndis.c
> > @@ -198,6 +198,9 @@ static int gen_ndis_query_resp(struct
> > rndis_params *params, u32 OID, u8 *buf,
> >       outbuf = (__le32 *)&resp[1];
> >       resp->InformationBufferOffset = cpu_to_le32(16);
> > 
> > +     if (!params->dev)
> > +             return -ENODEV;
> > +
> 
> As Sergey points out, this check is useless and the ones below should
> also be removed.
> 
> But, why make this check at all, how did you trigger a problem with
> the
> current code?
> 
> Are you using this driver?  If so, why?  It is totally broken (as per
> the specification) and we really really need to just delete it from
> the
> tree to prevent anyone else from ever using it.
> 
> thanks,
> 
> greg k-h
Thanks for review, please ignore this patch.
Issue was triggered by our internal tool (probably not smart enough,
and cause false alarm).

--nvpublic
Greg Kroah-Hartman Sept. 9, 2022, 6:07 p.m. UTC | #4 
On Fri, Sep 09, 2022 at 02:43:14PM +0000, Jim Lin wrote:
> On Fri, 2022-09-09 at 07:38 +0200, Greg KH wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Fri, Sep 09, 2022 at 01:56:15AM +0800, Jim Lin wrote:
> > > NULL check is performed after params->dev is dereferenced in
> > > dev_get_stats.
> > 
> > I do not understand this statement.
> > 
> > > Fixed by adding a NULL check before dereferencing params->dev and
> > > removing subsequent NULL checks for it.
> > > 
> > > Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
> > > Signed-off-by: Jim Lin <jilin@nvidia.com>
> > > ---
> > >  drivers/usb/gadget/function/rndis.c | 37 ++++++++++++-------------
> > > ----
> > >  1 file changed, 15 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/usb/gadget/function/rndis.c
> > > b/drivers/usb/gadget/function/rndis.c
> > > index 64de9f1b874c..d2f18f34c8e5 100644
> > > --- a/drivers/usb/gadget/function/rndis.c
> > > +++ b/drivers/usb/gadget/function/rndis.c
> > > @@ -198,6 +198,9 @@ static int gen_ndis_query_resp(struct
> > > rndis_params *params, u32 OID, u8 *buf,
> > >       outbuf = (__le32 *)&resp[1];
> > >       resp->InformationBufferOffset = cpu_to_le32(16);
> > > 
> > > +     if (!params->dev)
> > > +             return -ENODEV;
> > > +
> > 
> > As Sergey points out, this check is useless and the ones below should
> > also be removed.
> > 
> > But, why make this check at all, how did you trigger a problem with
> > the
> > current code?
> > 
> > Are you using this driver?  If so, why?  It is totally broken (as per
> > the specification) and we really really need to just delete it from
> > the
> > tree to prevent anyone else from ever using it.
> > 
> > thanks,
> > 
> > greg k-h
> Thanks for review, please ignore this patch.
> Issue was triggered by our internal tool (probably not smart enough,
> and cause false alarm).

Ah, so that means I HAVE to reject it as you did not properly follow the
requirements when you use tools like this.  Please read
Documentation/process/researcher-guidelines.rst for how to do this
properly.

That is now a requirement to make it so that we are not the ones doing
the debugging of random tools like we just had to do here...

You owe me 3 patch reviews now :)

thanks,

greg k-h
Sergey Shtylyov Sept. 22, 2022, 9 p.m. UTC | #5 
Hello!

On 9/9/22 8:38 AM, Greg KH wrote:

>> NULL check is performed after params->dev is dereferenced in
>> dev_get_stats.
> 
> I do not understand this statement.
> 
>> Fixed by adding a NULL check before dereferencing params->dev and
>> removing subsequent NULL checks for it.
>>
>> Signed-off-by: Aniruddha TVS Rao <anrao@nvidia.com>
>> Signed-off-by: Jim Lin <jilin@nvidia.com>
>> ---
>>  drivers/usb/gadget/function/rndis.c | 37 ++++++++++++-----------------
>>  1 file changed, 15 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
>> index 64de9f1b874c..d2f18f34c8e5 100644
>> --- a/drivers/usb/gadget/function/rndis.c
>> +++ b/drivers/usb/gadget/function/rndis.c
>> @@ -198,6 +198,9 @@ static int gen_ndis_query_resp(struct rndis_params *params, u32 OID, u8 *buf,
>>  	outbuf = (__le32 *)&resp[1];
>>  	resp->InformationBufferOffset = cpu_to_le32(16);
>>  
>> +	if (!params->dev)
>> +		return -ENODEV;
>> +
> 
> As Sergey points out, this check is useless and the ones below should
> also be removed.

   Would you accept this patch modulo the above check then? If not,
I'll just resolve the corresponding SVACE checker as "won't fix" here. :-)

> But, why make this check at all, how did you trigger a problem with the
> current code?

   There's no problem, just the redundant NULL checks...

> Are you using this driver?  If so, why?  It is totally broken (as per
> the specification) and we really really need to just delete it from the
> tree to prevent anyone else from ever using it.

   Well, delete it then... :-)

> thanks,
> 
> greg k-h

MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
index 64de9f1b874c..d2f18f34c8e5 100644
--- a/drivers/usb/gadget/function/rndis.c
+++ b/drivers/usb/gadget/function/rndis.c
@@ -198,6 +198,9 @@  static int gen_ndis_query_resp(struct rndis_params *params, u32 OID, u8 *buf,
 	outbuf = (__le32 *)&resp[1];
 	resp->InformationBufferOffset = cpu_to_le32(16);
 
+	if (!params->dev)
+		return -ENODEV;
+
 	net = params->dev;
 	stats = dev_get_stats(net, &temp);
 
@@ -246,10 +249,8 @@  static int gen_ndis_query_resp(struct rndis_params *params, u32 OID, u8 *buf,
 	/* mandatory */
 	case RNDIS_OID_GEN_MAXIMUM_FRAME_SIZE:
 		pr_debug("%s: RNDIS_OID_GEN_MAXIMUM_FRAME_SIZE\n", __func__);
-		if (params->dev) {
-			*outbuf = cpu_to_le32(params->dev->mtu);
-			retval = 0;
-		}
+		*outbuf = cpu_to_le32(params->dev->mtu);
+		retval = 0;
 		break;
 
 	/* mandatory */
@@ -266,19 +267,15 @@  static int gen_ndis_query_resp(struct rndis_params *params, u32 OID, u8 *buf,
 	/* mandatory */
 	case RNDIS_OID_GEN_TRANSMIT_BLOCK_SIZE:
 		pr_debug("%s: RNDIS_OID_GEN_TRANSMIT_BLOCK_SIZE\n", __func__);
-		if (params->dev) {
-			*outbuf = cpu_to_le32(params->dev->mtu);
-			retval = 0;
-		}
+		*outbuf = cpu_to_le32(params->dev->mtu);
+		retval = 0;
 		break;
 
 	/* mandatory */
 	case RNDIS_OID_GEN_RECEIVE_BLOCK_SIZE:
 		pr_debug("%s: RNDIS_OID_GEN_RECEIVE_BLOCK_SIZE\n", __func__);
-		if (params->dev) {
-			*outbuf = cpu_to_le32(params->dev->mtu);
-			retval = 0;
-		}
+		*outbuf = cpu_to_le32(params->dev->mtu);
+		retval = 0;
 		break;
 
 	/* mandatory */
@@ -405,21 +402,17 @@  static int gen_ndis_query_resp(struct rndis_params *params, u32 OID, u8 *buf,
 	/* mandatory */
 	case RNDIS_OID_802_3_PERMANENT_ADDRESS:
 		pr_debug("%s: RNDIS_OID_802_3_PERMANENT_ADDRESS\n", __func__);
-		if (params->dev) {
-			length = ETH_ALEN;
-			memcpy(outbuf, params->host_mac, length);
-			retval = 0;
-		}
+		length = ETH_ALEN;
+		memcpy(outbuf, params->host_mac, length);
+		retval = 0;
 		break;
 
 	/* mandatory */
 	case RNDIS_OID_802_3_CURRENT_ADDRESS:
 		pr_debug("%s: RNDIS_OID_802_3_CURRENT_ADDRESS\n", __func__);
-		if (params->dev) {
-			length = ETH_ALEN;
-			memcpy(outbuf, params->host_mac, length);
-			retval = 0;
-		}
+		length = ETH_ALEN;
+		memcpy(outbuf, params->host_mac, length);
+		retval = 0;
 		break;
 
 	/* mandatory */