* [PATCH] avrcp: Fix wrong pointer check @ 2014-01-31 8:33 Andrei Emeltchenko 2014-02-02 16:03 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 6+ messages in thread From: Andrei Emeltchenko @ 2014-01-31 8:33 UTC (permalink / raw) To: linux-bluetooth From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> There is wrong assumption that handler might be NULL while it is a pointer to a struct table so check instead for struct members. This fixes accessing wrong memory. --- profiles/audio/avrcp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c index df88138..5030ce1 100644 --- a/profiles/audio/avrcp.c +++ b/profiles/audio/avrcp.c @@ -1673,7 +1673,7 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction, break; } - if (!handler || handler->code != *code) { + if (!handler->code || handler->code != *code) { pdu->params[0] = AVRCP_STATUS_INVALID_COMMAND; goto err_metadata; } @@ -1737,7 +1737,7 @@ static size_t handle_browsing_pdu(struct avctp *conn, break; } - if (handler == NULL || handler->func == NULL) + if (!handler->func) return avrcp_browsing_general_reject(operands); session->transaction = transaction; -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] avrcp: Fix wrong pointer check 2014-01-31 8:33 [PATCH] avrcp: Fix wrong pointer check Andrei Emeltchenko @ 2014-02-02 16:03 ` Luiz Augusto von Dentz 2014-02-03 7:51 ` Andrei Emeltchenko 0 siblings, 1 reply; 6+ messages in thread From: Luiz Augusto von Dentz @ 2014-02-02 16:03 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth@vger.kernel.org Hi Andrei, On Fri, Jan 31, 2014 at 12:33 AM, Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> wrote: > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > There is wrong assumption that handler might be NULL while it is a > pointer to a struct table so check instead for struct members. This > fixes accessing wrong memory. > --- > profiles/audio/avrcp.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c > index df88138..5030ce1 100644 > --- a/profiles/audio/avrcp.c > +++ b/profiles/audio/avrcp.c > @@ -1673,7 +1673,7 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction, > break; > } > > - if (!handler || handler->code != *code) { > + if (!handler->code || handler->code != *code) { The code checks if session->control_handlers is initialized and Im not sure what is the invalid memory access you are talking about since handle->code is no a pointer, I do agree that we should probably drop the second check for the handler in the lines bellow. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] avrcp: Fix wrong pointer check 2014-02-02 16:03 ` Luiz Augusto von Dentz @ 2014-02-03 7:51 ` Andrei Emeltchenko 2014-02-03 9:20 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 6+ messages in thread From: Andrei Emeltchenko @ 2014-02-03 7:51 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org Hi Luiz, On Sun, Feb 02, 2014 at 08:03:34AM -0800, Luiz Augusto von Dentz wrote: > Hi Andrei, > > On Fri, Jan 31, 2014 at 12:33 AM, Andrei Emeltchenko > <Andrei.Emeltchenko.news@gmail.com> wrote: > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > > > There is wrong assumption that handler might be NULL while it is a > > pointer to a struct table so check instead for struct members. This > > fixes accessing wrong memory. > > --- > > profiles/audio/avrcp.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c > > index df88138..5030ce1 100644 > > --- a/profiles/audio/avrcp.c > > +++ b/profiles/audio/avrcp.c > > @@ -1673,7 +1673,7 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction, > > break; > > } > > > > - if (!handler || handler->code != *code) { > > + if (!handler->code || handler->code != *code) { > > The code checks if session->control_handlers is initialized and Im > not sure what is the invalid memory access you are talking about since > handle->code is no a pointer, I do agree that we should probably drop > the second check for the handler in the lines bellow. handler is a pointer, and it points to static const struct control_pdu_handler control_handlers[] table with the last element: ... { }, }; ... So checking for !handler is pointless. Best regards Andrei Emeltchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] avrcp: Fix wrong pointer check 2014-02-03 7:51 ` Andrei Emeltchenko @ 2014-02-03 9:20 ` Luiz Augusto von Dentz 2014-02-03 9:36 ` Andrei Emeltchenko 0 siblings, 1 reply; 6+ messages in thread From: Luiz Augusto von Dentz @ 2014-02-03 9:20 UTC (permalink / raw) To: Andrei Emeltchenko, Luiz Augusto von Dentz, linux-bluetooth@vger.kernel.org Hi Andrei, On Sun, Feb 2, 2014 at 11:51 PM, Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> wrote: > Hi Luiz, > > On Sun, Feb 02, 2014 at 08:03:34AM -0800, Luiz Augusto von Dentz wrote: >> Hi Andrei, >> >> On Fri, Jan 31, 2014 at 12:33 AM, Andrei Emeltchenko >> <Andrei.Emeltchenko.news@gmail.com> wrote: >> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> >> > >> > There is wrong assumption that handler might be NULL while it is a >> > pointer to a struct table so check instead for struct members. This >> > fixes accessing wrong memory. >> > --- >> > profiles/audio/avrcp.c | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c >> > index df88138..5030ce1 100644 >> > --- a/profiles/audio/avrcp.c >> > +++ b/profiles/audio/avrcp.c >> > @@ -1673,7 +1673,7 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction, >> > break; >> > } >> > >> > - if (!handler || handler->code != *code) { >> > + if (!handler->code || handler->code != *code) { >> >> The code checks if session->control_handlers is initialized and Im >> not sure what is the invalid memory access you are talking about since >> handle->code is no a pointer, I do agree that we should probably drop >> the second check for the handler in the lines bellow. > > handler is a pointer, and it points to > > static const struct control_pdu_handler control_handlers[] > table with the last element: > > ... > { }, > }; > ... > > So checking for !handler is pointless. Right, because checking for pointer is pointless, yes Im being sarcastic here... Now lets be clear, you are changing a check of a pointer to a value and claiming it fixes invalid accesses which does not make any sense, what could make sense is to check if handler->pdu_id == pdu->pdu_id since that what we check when we lookup for a handle. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] avrcp: Fix wrong pointer check 2014-02-03 9:20 ` Luiz Augusto von Dentz @ 2014-02-03 9:36 ` Andrei Emeltchenko 2014-02-03 9:42 ` Andrei Emeltchenko 0 siblings, 1 reply; 6+ messages in thread From: Andrei Emeltchenko @ 2014-02-03 9:36 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org On Mon, Feb 03, 2014 at 01:20:11AM -0800, Luiz Augusto von Dentz wrote: > Hi Andrei, > > On Sun, Feb 2, 2014 at 11:51 PM, Andrei Emeltchenko > <Andrei.Emeltchenko.news@gmail.com> wrote: > > Hi Luiz, > > > > On Sun, Feb 02, 2014 at 08:03:34AM -0800, Luiz Augusto von Dentz wrote: > >> Hi Andrei, > >> > >> On Fri, Jan 31, 2014 at 12:33 AM, Andrei Emeltchenko > >> <Andrei.Emeltchenko.news@gmail.com> wrote: > >> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > >> > > >> > There is wrong assumption that handler might be NULL while it is a > >> > pointer to a struct table so check instead for struct members. This > >> > fixes accessing wrong memory. > >> > --- > >> > profiles/audio/avrcp.c | 4 ++-- > >> > 1 file changed, 2 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c > >> > index df88138..5030ce1 100644 > >> > --- a/profiles/audio/avrcp.c > >> > +++ b/profiles/audio/avrcp.c > >> > @@ -1673,7 +1673,7 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction, > >> > break; > >> > } > >> > > >> > - if (!handler || handler->code != *code) { > >> > + if (!handler->code || handler->code != *code) { > >> > >> The code checks if session->control_handlers is initialized and Im > >> not sure what is the invalid memory access you are talking about since > >> handle->code is no a pointer, I do agree that we should probably drop > >> the second check for the handler in the lines bellow. > > > > handler is a pointer, and it points to > > > > static const struct control_pdu_handler control_handlers[] > > table with the last element: > > > > ... > > { }, > > }; > > ... > > > > So checking for !handler is pointless. > > Right, because checking for pointer is pointless, yes Im being > sarcastic here... Now lets be clear, you are changing a check of a > pointer to a value and claiming it fixes invalid accesses which does > not make any sense, what could make sense is to check if > handler->pdu_id == pdu->pdu_id since that what we check when we lookup > for a handle. No, we break if handler->pdu_id == pdu->pdu_id one line above ;) Best regards Andrei Emeltchenko > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] avrcp: Fix wrong pointer check 2014-02-03 9:36 ` Andrei Emeltchenko @ 2014-02-03 9:42 ` Andrei Emeltchenko 0 siblings, 0 replies; 6+ messages in thread From: Andrei Emeltchenko @ 2014-02-03 9:42 UTC (permalink / raw) To: Luiz Augusto von Dentz, linux-bluetooth@vger.kernel.org On Mon, Feb 03, 2014 at 11:36:52AM +0200, Andrei Emeltchenko wrote: > On Mon, Feb 03, 2014 at 01:20:11AM -0800, Luiz Augusto von Dentz wrote: > > Hi Andrei, > > > > On Sun, Feb 2, 2014 at 11:51 PM, Andrei Emeltchenko > > <Andrei.Emeltchenko.news@gmail.com> wrote: > > > Hi Luiz, > > > > > > On Sun, Feb 02, 2014 at 08:03:34AM -0800, Luiz Augusto von Dentz wrote: > > >> Hi Andrei, > > >> > > >> On Fri, Jan 31, 2014 at 12:33 AM, Andrei Emeltchenko > > >> <Andrei.Emeltchenko.news@gmail.com> wrote: > > >> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > >> > > > >> > There is wrong assumption that handler might be NULL while it is a > > >> > pointer to a struct table so check instead for struct members. This > > >> > fixes accessing wrong memory. > > >> > --- > > >> > profiles/audio/avrcp.c | 4 ++-- > > >> > 1 file changed, 2 insertions(+), 2 deletions(-) > > >> > > > >> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c > > >> > index df88138..5030ce1 100644 > > >> > --- a/profiles/audio/avrcp.c > > >> > +++ b/profiles/audio/avrcp.c > > >> > @@ -1673,7 +1673,7 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction, > > >> > break; > > >> > } > > >> > > > >> > - if (!handler || handler->code != *code) { > > >> > + if (!handler->code || handler->code != *code) { > > >> > > >> The code checks if session->control_handlers is initialized and Im > > >> not sure what is the invalid memory access you are talking about since > > >> handle->code is no a pointer, I do agree that we should probably drop > > >> the second check for the handler in the lines bellow. > > > > > > handler is a pointer, and it points to > > > > > > static const struct control_pdu_handler control_handlers[] > > > table with the last element: > > > > > > ... > > > { }, > > > }; > > > ... > > > > > > So checking for !handler is pointless. > > > > Right, because checking for pointer is pointless, yes Im being > > sarcastic here... Now lets be clear, you are changing a check of a > > pointer to a value and claiming it fixes invalid accesses which does > > not make any sense, what could make sense is to check if > > handler->pdu_id == pdu->pdu_id since that what we check when we lookup > > for a handle. > > No, we break if handler->pdu_id == pdu->pdu_id one line above ;) Sorry this was wrong. So my idea is to check that we are not at the end of the table. In this case every element of a struct is zero, so it does not matter which one to check. Best regards Andrei Emeltchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-02-03 9:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-31 8:33 [PATCH] avrcp: Fix wrong pointer check Andrei Emeltchenko 2014-02-02 16:03 ` Luiz Augusto von Dentz 2014-02-03 7:51 ` Andrei Emeltchenko 2014-02-03 9:20 ` Luiz Augusto von Dentz 2014-02-03 9:36 ` Andrei Emeltchenko 2014-02-03 9:42 ` Andrei Emeltchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox