* [PATCH] media: flexcop-usb: fix sanity check of bNumEndpoints
@ 2022-06-02 5:50 Dongliang Mu
2022-06-02 6:45 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Dongliang Mu @ 2022-06-02 5:50 UTC (permalink / raw)
To: Mauro Carvalho Chehab, gushengxian, Hans Verkuil, Johan Hovold,
Oliver Neukum, Sean Young
Cc: Dongliang Mu, Mauro Carvalho Chehab, linux-media, linux-kernel
From: Dongliang Mu <mudongliangabcd@gmail.com>
Commit d725d20e81c2 ("media: flexcop-usb: sanity checking of endpoint type
") adds a sanity check for endpoint[1], but fails to modify the sanity
check of bNumEndpoints.
Fix this by modifying the sanity check of bNumEndpoints to 2.
Fixes: d725d20e81c2 ("media: flexcop-usb: sanity checking of endpoint type")
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
drivers/media/usb/b2c2/flexcop-usb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c
index 7835bb0f32fc..eb54bfdda01f 100644
--- a/drivers/media/usb/b2c2/flexcop-usb.c
+++ b/drivers/media/usb/b2c2/flexcop-usb.c
@@ -509,7 +509,7 @@ static int flexcop_usb_init(struct flexcop_usb *fc_usb)
return ret;
}
- if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 1)
+ if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 2)
return -ENODEV;
if (!usb_endpoint_is_isoc_in(&fc_usb->uintf->cur_altsetting->endpoint[1].desc))
return -ENODEV;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] media: flexcop-usb: fix sanity check of bNumEndpoints
2022-06-02 5:50 [PATCH] media: flexcop-usb: fix sanity check of bNumEndpoints Dongliang Mu
@ 2022-06-02 6:45 ` Johan Hovold
2022-06-09 1:42 ` Dongliang Mu
0 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2022-06-02 6:45 UTC (permalink / raw)
To: Dongliang Mu
Cc: Mauro Carvalho Chehab, gushengxian, Hans Verkuil, Oliver Neukum,
Sean Young, Dongliang Mu, Mauro Carvalho Chehab, linux-media,
linux-kernel
On Thu, Jun 02, 2022 at 01:50:24PM +0800, Dongliang Mu wrote:
> From: Dongliang Mu <mudongliangabcd@gmail.com>
>
> Commit d725d20e81c2 ("media: flexcop-usb: sanity checking of endpoint type
> ") adds a sanity check for endpoint[1], but fails to modify the sanity
> check of bNumEndpoints.
>
> Fix this by modifying the sanity check of bNumEndpoints to 2.
Are you sure that d725d20e81c2 ("media: flexcop-usb: sanity checking of
endpoint type) was correct?
> Fixes: d725d20e81c2 ("media: flexcop-usb: sanity checking of endpoint type")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
> drivers/media/usb/b2c2/flexcop-usb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c
> index 7835bb0f32fc..eb54bfdda01f 100644
> --- a/drivers/media/usb/b2c2/flexcop-usb.c
> +++ b/drivers/media/usb/b2c2/flexcop-usb.c
> @@ -509,7 +509,7 @@ static int flexcop_usb_init(struct flexcop_usb *fc_usb)
> return ret;
> }
>
> - if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 1)
> + if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 2)
> return -ENODEV;
> if (!usb_endpoint_is_isoc_in(&fc_usb->uintf->cur_altsetting->endpoint[1].desc))
> return -ENODEV;
It looks like it should have verified the type of endpoint[0] here
instead.
Oliver?
Johan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: flexcop-usb: fix sanity check of bNumEndpoints
2022-06-02 6:45 ` Johan Hovold
@ 2022-06-09 1:42 ` Dongliang Mu
2022-06-09 14:00 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Dongliang Mu @ 2022-06-09 1:42 UTC (permalink / raw)
To: Johan Hovold
Cc: Dongliang Mu, Mauro Carvalho Chehab, gushengxian, Hans Verkuil,
Oliver Neukum, Sean Young, Mauro Carvalho Chehab, linux-media,
linux-kernel
On Thu, Jun 2, 2022 at 2:45 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Jun 02, 2022 at 01:50:24PM +0800, Dongliang Mu wrote:
> > From: Dongliang Mu <mudongliangabcd@gmail.com>
> >
> > Commit d725d20e81c2 ("media: flexcop-usb: sanity checking of endpoint type
> > ") adds a sanity check for endpoint[1], but fails to modify the sanity
> > check of bNumEndpoints.
> >
> > Fix this by modifying the sanity check of bNumEndpoints to 2.
>
> Are you sure that d725d20e81c2 ("media: flexcop-usb: sanity checking of
> endpoint type) was correct?
I am not sure if fc_usb->uintf->cur_altsetting->endpoint[1] is right or not.
But the sanity check of bNumEndpoints does not match the following dereference.
>
> > Fixes: d725d20e81c2 ("media: flexcop-usb: sanity checking of endpoint type")
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> > drivers/media/usb/b2c2/flexcop-usb.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c
> > index 7835bb0f32fc..eb54bfdda01f 100644
> > --- a/drivers/media/usb/b2c2/flexcop-usb.c
> > +++ b/drivers/media/usb/b2c2/flexcop-usb.c
> > @@ -509,7 +509,7 @@ static int flexcop_usb_init(struct flexcop_usb *fc_usb)
> > return ret;
> > }
> >
> > - if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 1)
> > + if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 2)
> > return -ENODEV;
> > if (!usb_endpoint_is_isoc_in(&fc_usb->uintf->cur_altsetting->endpoint[1].desc))
> > return -ENODEV;
>
> It looks like it should have verified the type of endpoint[0] here
> instead.
This sanity check originally makes sure the
fc_usb->uintf->cur_altsetting->endpoint[0] in
flexcop_usb_transfer_init.
>
> Oliver?
>
> Johan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: flexcop-usb: fix sanity check of bNumEndpoints
2022-06-09 1:42 ` Dongliang Mu
@ 2022-06-09 14:00 ` Johan Hovold
2022-06-10 1:27 ` Dongliang Mu
0 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2022-06-09 14:00 UTC (permalink / raw)
To: Dongliang Mu
Cc: Dongliang Mu, Mauro Carvalho Chehab, gushengxian, Hans Verkuil,
Oliver Neukum, Sean Young, Mauro Carvalho Chehab, linux-media,
linux-kernel
On Thu, Jun 09, 2022 at 09:42:15AM +0800, Dongliang Mu wrote:
> On Thu, Jun 2, 2022 at 2:45 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Jun 02, 2022 at 01:50:24PM +0800, Dongliang Mu wrote:
> > > From: Dongliang Mu <mudongliangabcd@gmail.com>
> > >
> > > Commit d725d20e81c2 ("media: flexcop-usb: sanity checking of endpoint type
> > > ") adds a sanity check for endpoint[1], but fails to modify the sanity
> > > check of bNumEndpoints.
> > >
> > > Fix this by modifying the sanity check of bNumEndpoints to 2.
> >
> > Are you sure that d725d20e81c2 ("media: flexcop-usb: sanity checking of
> > endpoint type) was correct?
>
> I am not sure if fc_usb->uintf->cur_altsetting->endpoint[1] is right or not.
>
> But the sanity check of bNumEndpoints does not match the following dereference.
I should know better than try to be subtle on the internet: Your fix is
wrong.
You need to try to understand the code you're changing. Not just do
pattern matching.
> > > Fixes: d725d20e81c2 ("media: flexcop-usb: sanity checking of endpoint type")
> > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > ---
> > > drivers/media/usb/b2c2/flexcop-usb.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c
> > > index 7835bb0f32fc..eb54bfdda01f 100644
> > > --- a/drivers/media/usb/b2c2/flexcop-usb.c
> > > +++ b/drivers/media/usb/b2c2/flexcop-usb.c
> > > @@ -509,7 +509,7 @@ static int flexcop_usb_init(struct flexcop_usb *fc_usb)
> > > return ret;
> > > }
> > >
> > > - if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 1)
> > > + if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 2)
> > > return -ENODEV;
> > > if (!usb_endpoint_is_isoc_in(&fc_usb->uintf->cur_altsetting->endpoint[1].desc))
> > > return -ENODEV;
> >
> > It looks like it should have verified the type of endpoint[0] here
> > instead.
>
> This sanity check originally makes sure the
> fc_usb->uintf->cur_altsetting->endpoint[0] in
> flexcop_usb_transfer_init.
Yes, and that is the only endpoint that this driver uses.
I've posted a fix here:
https://lore.kernel.org/r/20220609135341.19941-1-johan@kernel.org
Johan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: flexcop-usb: fix sanity check of bNumEndpoints
2022-06-09 14:00 ` Johan Hovold
@ 2022-06-10 1:27 ` Dongliang Mu
2022-06-10 7:29 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Dongliang Mu @ 2022-06-10 1:27 UTC (permalink / raw)
To: Johan Hovold
Cc: Dongliang Mu, Mauro Carvalho Chehab, gushengxian, Hans Verkuil,
Oliver Neukum, Sean Young, Mauro Carvalho Chehab, linux-media,
linux-kernel
On Thu, Jun 9, 2022 at 10:00 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Jun 09, 2022 at 09:42:15AM +0800, Dongliang Mu wrote:
> > On Thu, Jun 2, 2022 at 2:45 PM Johan Hovold <johan@kernel.org> wrote:
> > >
> > > On Thu, Jun 02, 2022 at 01:50:24PM +0800, Dongliang Mu wrote:
> > > > From: Dongliang Mu <mudongliangabcd@gmail.com>
> > > >
> > > > Commit d725d20e81c2 ("media: flexcop-usb: sanity checking of endpoint type
> > > > ") adds a sanity check for endpoint[1], but fails to modify the sanity
> > > > check of bNumEndpoints.
> > > >
> > > > Fix this by modifying the sanity check of bNumEndpoints to 2.
> > >
> > > Are you sure that d725d20e81c2 ("media: flexcop-usb: sanity checking of
> > > endpoint type) was correct?
> >
> > I am not sure if fc_usb->uintf->cur_altsetting->endpoint[1] is right or not.
> >
> > But the sanity check of bNumEndpoints does not match the following dereference.
>
> I should know better than try to be subtle on the internet: Your fix is
> wrong.
>
> You need to try to understand the code you're changing. Not just do
> pattern matching.
I see. Yeah, I have such an incorrect assumption - previous commits
should be somehow correct since maintainers already review them.
Next time, I will dig more about the code logic and try to post a more
comprehensive patch.
>
> > > > Fixes: d725d20e81c2 ("media: flexcop-usb: sanity checking of endpoint type")
> > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > > ---
> > > > drivers/media/usb/b2c2/flexcop-usb.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c
> > > > index 7835bb0f32fc..eb54bfdda01f 100644
> > > > --- a/drivers/media/usb/b2c2/flexcop-usb.c
> > > > +++ b/drivers/media/usb/b2c2/flexcop-usb.c
> > > > @@ -509,7 +509,7 @@ static int flexcop_usb_init(struct flexcop_usb *fc_usb)
> > > > return ret;
> > > > }
> > > >
> > > > - if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 1)
> > > > + if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 2)
> > > > return -ENODEV;
> > > > if (!usb_endpoint_is_isoc_in(&fc_usb->uintf->cur_altsetting->endpoint[1].desc))
> > > > return -ENODEV;
> > >
> > > It looks like it should have verified the type of endpoint[0] here
> > > instead.
> >
> > This sanity check originally makes sure the
> > fc_usb->uintf->cur_altsetting->endpoint[0] in
> > flexcop_usb_transfer_init.
>
> Yes, and that is the only endpoint that this driver uses.
This needs an understanding of the working mechanism of this hardware.
>
> I've posted a fix here:
>
> https://lore.kernel.org/r/20220609135341.19941-1-johan@kernel.org
>
> Johan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: flexcop-usb: fix sanity check of bNumEndpoints
2022-06-10 1:27 ` Dongliang Mu
@ 2022-06-10 7:29 ` Johan Hovold
0 siblings, 0 replies; 6+ messages in thread
From: Johan Hovold @ 2022-06-10 7:29 UTC (permalink / raw)
To: Dongliang Mu
Cc: Dongliang Mu, Mauro Carvalho Chehab, gushengxian, Hans Verkuil,
Oliver Neukum, Sean Young, Mauro Carvalho Chehab, linux-media,
linux-kernel
On Fri, Jun 10, 2022 at 09:27:27AM +0800, Dongliang Mu wrote:
> On Thu, Jun 9, 2022 at 10:00 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Jun 09, 2022 at 09:42:15AM +0800, Dongliang Mu wrote:
> > > On Thu, Jun 2, 2022 at 2:45 PM Johan Hovold <johan@kernel.org> wrote:
> > > > Are you sure that d725d20e81c2 ("media: flexcop-usb: sanity checking of
> > > > endpoint type) was correct?
> > >
> > > I am not sure if fc_usb->uintf->cur_altsetting->endpoint[1] is right or not.
> > >
> > > But the sanity check of bNumEndpoints does not match the following dereference.
> >
> > I should know better than try to be subtle on the internet: Your fix is
> > wrong.
> >
> > You need to try to understand the code you're changing. Not just do
> > pattern matching.
>
> I see. Yeah, I have such an incorrect assumption - previous commits
> should be somehow correct since maintainers already review them.
Everyone makes mistakes and our review process isn't perfect.
And if your assumption was correct we wouldn't have any bugs at all.
> Next time, I will dig more about the code logic and try to post a more
> comprehensive patch.
Sounds good.
> > > > > Fixes: d725d20e81c2 ("media: flexcop-usb: sanity checking of endpoint type")
> > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > > > ---
> > > > > drivers/media/usb/b2c2/flexcop-usb.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c
> > > > > index 7835bb0f32fc..eb54bfdda01f 100644
> > > > > --- a/drivers/media/usb/b2c2/flexcop-usb.c
> > > > > +++ b/drivers/media/usb/b2c2/flexcop-usb.c
> > > > > @@ -509,7 +509,7 @@ static int flexcop_usb_init(struct flexcop_usb *fc_usb)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > - if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 1)
> > > > > + if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints < 2)
> > > > > return -ENODEV;
> > > > > if (!usb_endpoint_is_isoc_in(&fc_usb->uintf->cur_altsetting->endpoint[1].desc))
> > > > > return -ENODEV;
> > > >
> > > > It looks like it should have verified the type of endpoint[0] here
> > > > instead.
> > >
> > > This sanity check originally makes sure the
> > > fc_usb->uintf->cur_altsetting->endpoint[0] in
> > > flexcop_usb_transfer_init.
> >
> > Yes, and that is the only endpoint that this driver uses.
>
> This needs an understanding of the working mechanism of this hardware.
Not really, you only need to look at the driver in this case. But sure,
you need some familiarity with USB to understand what is going on.
Johan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-10 7:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-02 5:50 [PATCH] media: flexcop-usb: fix sanity check of bNumEndpoints Dongliang Mu
2022-06-02 6:45 ` Johan Hovold
2022-06-09 1:42 ` Dongliang Mu
2022-06-09 14:00 ` Johan Hovold
2022-06-10 1:27 ` Dongliang Mu
2022-06-10 7:29 ` Johan Hovold
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).