* [PATCH] usb: gadget: rndis: Avoid dereference before NULL check
@ 2022-09-08 17:56 Jim Lin
2022-09-08 20:30 ` Sergey Shtylyov
2022-09-09 5:38 ` Greg KH
0 siblings, 2 replies; 6+ messages in thread
From: Jim Lin @ 2022-09-08 17:56 UTC (permalink / raw)
To: balbi, gregkh; +Cc: linux-usb, linux-kernel, Jim Lin, Aniruddha TVS Rao
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(-)
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 */
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] usb: gadget: rndis: Avoid dereference before NULL check
2022-09-08 17:56 [PATCH] usb: gadget: rndis: Avoid dereference before NULL check Jim Lin
@ 2022-09-08 20:30 ` Sergey Shtylyov
2022-09-09 5:38 ` Greg KH
1 sibling, 0 replies; 6+ messages in thread
From: Sergey Shtylyov @ 2022-09-08 20:30 UTC (permalink / raw)
To: Jim Lin, balbi, gregkh; +Cc: linux-usb, linux-kernel, Aniruddha TVS Rao
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: rndis: Avoid dereference before NULL check
2022-09-08 17:56 [PATCH] usb: gadget: rndis: Avoid dereference before NULL check Jim Lin
2022-09-08 20:30 ` Sergey Shtylyov
@ 2022-09-09 5:38 ` Greg KH
2022-09-09 14:43 ` Jim Lin
2022-09-22 21:00 ` Sergei Shtylyov
1 sibling, 2 replies; 6+ messages in thread
From: Greg KH @ 2022-09-09 5:38 UTC (permalink / raw)
To: Jim Lin; +Cc: balbi, linux-usb, linux-kernel, Aniruddha TVS Rao
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: rndis: Avoid dereference before NULL check
2022-09-09 5:38 ` Greg KH
@ 2022-09-09 14:43 ` Jim Lin
2022-09-09 18:07 ` gregkh
2022-09-22 21:00 ` Sergei Shtylyov
1 sibling, 1 reply; 6+ messages in thread
From: Jim Lin @ 2022-09-09 14:43 UTC (permalink / raw)
To: s.shtylyov, gregkh; +Cc: linux-kernel, Aniruddha Tvs Rao, balbi, linux-usb
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: rndis: Avoid dereference before NULL check
2022-09-09 14:43 ` Jim Lin
@ 2022-09-09 18:07 ` gregkh
0 siblings, 0 replies; 6+ messages in thread
From: gregkh @ 2022-09-09 18:07 UTC (permalink / raw)
To: Jim Lin; +Cc: s.shtylyov, linux-kernel, Aniruddha Tvs Rao, balbi, linux-usb
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: rndis: Avoid dereference before NULL check
2022-09-09 5:38 ` Greg KH
2022-09-09 14:43 ` Jim Lin
@ 2022-09-22 21:00 ` Sergei Shtylyov
1 sibling, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2022-09-22 21:00 UTC (permalink / raw)
To: Greg KH, Jim Lin; +Cc: balbi, linux-usb, linux-kernel, Aniruddha TVS Rao
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-22 21:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-08 17:56 [PATCH] usb: gadget: rndis: Avoid dereference before NULL check Jim Lin
2022-09-08 20:30 ` Sergey Shtylyov
2022-09-09 5:38 ` Greg KH
2022-09-09 14:43 ` Jim Lin
2022-09-09 18:07 ` gregkh
2022-09-22 21:00 ` Sergei Shtylyov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.