* [PATCH] nfc: st-nci: array index overflow in st_nci_se_get_bwi()
@ 2022-12-13 14:12 Aleksandr Burakov
2022-12-14 18:35 ` Alexander H Duyck
0 siblings, 1 reply; 5+ messages in thread
From: Aleksandr Burakov @ 2022-12-13 14:12 UTC (permalink / raw)
To: Krzysztof Kozlowski, Christophe Ricard, Samuel Ortiz
Cc: Aleksandr Burakov, netdev, linux-kernel, lvc-project
Index of info->se_info.atr can be overflow due to unchecked increment
in the loop "for". The patch checks the value of current array index
and doesn't permit increment in case of the index is equal to
ST_NCI_ESE_MAX_LENGTH - 1.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: ed06aeefdac3 ("nfc: st-nci: Rename st21nfcb to st-nci")
Signed-off-by: Aleksandr Burakov <a.burakov@rosalinux.ru>
---
drivers/nfc/st-nci/se.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
index ec87dd21e054..ff8ac1784880 100644
--- a/drivers/nfc/st-nci/se.c
+++ b/drivers/nfc/st-nci/se.c
@@ -119,10 +119,11 @@ static u8 st_nci_se_get_bwi(struct nci_dev *ndev)
/* Bits 8 to 5 of the first TB for T=1 encode BWI from zero to nine */
for (i = 1; i < ST_NCI_ESE_MAX_LENGTH; i++) {
td = ST_NCI_ATR_GET_Y_FROM_TD(info->se_info.atr[i]);
- if (ST_NCI_ATR_TA_PRESENT(td))
+ if (ST_NCI_ATR_TA_PRESENT(td) && i < ST_NCI_ESE_MAX_LENGTH - 1)
i++;
if (ST_NCI_ATR_TB_PRESENT(td)) {
- i++;
+ if (i < ST_NCI_ESE_MAX_LENGTH - 1)
+ i++;
return info->se_info.atr[i] >> 4;
}
}
--
2.35.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] nfc: st-nci: array index overflow in st_nci_se_get_bwi()
2022-12-13 14:12 [PATCH] nfc: st-nci: array index overflow in st_nci_se_get_bwi() Aleksandr Burakov
@ 2022-12-14 18:35 ` Alexander H Duyck
2022-12-19 9:06 ` Krzysztof Kozlowski
0 siblings, 1 reply; 5+ messages in thread
From: Alexander H Duyck @ 2022-12-14 18:35 UTC (permalink / raw)
To: Aleksandr Burakov, Krzysztof Kozlowski, Christophe Ricard, Samuel Ortiz
Cc: netdev, linux-kernel, lvc-project
On Tue, 2022-12-13 at 09:12 -0500, Aleksandr Burakov wrote:
> Index of info->se_info.atr can be overflow due to unchecked increment
> in the loop "for". The patch checks the value of current array index
> and doesn't permit increment in case of the index is equal to
> ST_NCI_ESE_MAX_LENGTH - 1.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: ed06aeefdac3 ("nfc: st-nci: Rename st21nfcb to st-nci")
> Signed-off-by: Aleksandr Burakov <a.burakov@rosalinux.ru>
> ---
> drivers/nfc/st-nci/se.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
> index ec87dd21e054..ff8ac1784880 100644
> --- a/drivers/nfc/st-nci/se.c
> +++ b/drivers/nfc/st-nci/se.c
> @@ -119,10 +119,11 @@ static u8 st_nci_se_get_bwi(struct nci_dev *ndev)
> /* Bits 8 to 5 of the first TB for T=1 encode BWI from zero to nine */
> for (i = 1; i < ST_NCI_ESE_MAX_LENGTH; i++) {
> td = ST_NCI_ATR_GET_Y_FROM_TD(info->se_info.atr[i]);
> - if (ST_NCI_ATR_TA_PRESENT(td))
> + if (ST_NCI_ATR_TA_PRESENT(td) && i < ST_NCI_ESE_MAX_LENGTH - 1)
> i++;
> if (ST_NCI_ATR_TB_PRESENT(td)) {
> - i++;
> + if (i < ST_NCI_ESE_MAX_LENGTH - 1)
> + i++;
> return info->se_info.atr[i] >> 4;
> }
> }
Rather than adding 2 checks you could do this all with one check.
Basically you would just need to replace:
if (ST_NCI_ATR_TB_PRESENT(td)) {
i++;
with:
if (ST_NCI_ATR_TB_PRESENT(td) && ++i < ST_NCI_ESE_MAX_LENGTH)
Basically it is fine to increment "i" as long as it isn't being used as
an index so just restricting the last access so that we don't
dereference using it as an index should be enough.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] nfc: st-nci: array index overflow in st_nci_se_get_bwi()
2022-12-14 18:35 ` Alexander H Duyck
@ 2022-12-19 9:06 ` Krzysztof Kozlowski
2022-12-19 15:41 ` Alexander Duyck
0 siblings, 1 reply; 5+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-19 9:06 UTC (permalink / raw)
To: Alexander H Duyck, Aleksandr Burakov, Christophe Ricard, Samuel Ortiz
Cc: netdev, linux-kernel, lvc-project
On 14/12/2022 19:35, Alexander H Duyck wrote:
> On Tue, 2022-12-13 at 09:12 -0500, Aleksandr Burakov wrote:
>> Index of info->se_info.atr can be overflow due to unchecked increment
>> in the loop "for". The patch checks the value of current array index
>> and doesn't permit increment in case of the index is equal to
>> ST_NCI_ESE_MAX_LENGTH - 1.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Fixes: ed06aeefdac3 ("nfc: st-nci: Rename st21nfcb to st-nci")
>> Signed-off-by: Aleksandr Burakov <a.burakov@rosalinux.ru>
>> ---
>> drivers/nfc/st-nci/se.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
>> index ec87dd21e054..ff8ac1784880 100644
>> --- a/drivers/nfc/st-nci/se.c
>> +++ b/drivers/nfc/st-nci/se.c
>> @@ -119,10 +119,11 @@ static u8 st_nci_se_get_bwi(struct nci_dev *ndev)
>> /* Bits 8 to 5 of the first TB for T=1 encode BWI from zero to nine */
>> for (i = 1; i < ST_NCI_ESE_MAX_LENGTH; i++) {
>> td = ST_NCI_ATR_GET_Y_FROM_TD(info->se_info.atr[i]);
>> - if (ST_NCI_ATR_TA_PRESENT(td))
>> + if (ST_NCI_ATR_TA_PRESENT(td) && i < ST_NCI_ESE_MAX_LENGTH - 1)
>> i++;
>> if (ST_NCI_ATR_TB_PRESENT(td)) {
>> - i++;
>> + if (i < ST_NCI_ESE_MAX_LENGTH - 1)
>> + i++;
>> return info->se_info.atr[i] >> 4;
>> }
>> }
>
> Rather than adding 2 checks you could do this all with one check.
> Basically you would just need to replace:
> if (ST_NCI_ATR_TB_PRESENT(td)) {
> i++;
>
> with:
> if (ST_NCI_ATR_TB_PRESENT(td) && ++i < ST_NCI_ESE_MAX_LENGTH)
>
> Basically it is fine to increment "i" as long as it isn't being used as
> an index so just restricting the last access so that we don't
> dereference using it as an index should be enough.
These are different checks - TA and TB. By skipping TA, your code is not
equivalent. Was it intended?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] nfc: st-nci: array index overflow in st_nci_se_get_bwi()
2022-12-19 9:06 ` Krzysztof Kozlowski
@ 2022-12-19 15:41 ` Alexander Duyck
2022-12-19 15:46 ` Krzysztof Kozlowski
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Duyck @ 2022-12-19 15:41 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Aleksandr Burakov, Christophe Ricard, Samuel Ortiz, netdev,
linux-kernel, lvc-project
On Mon, Dec 19, 2022 at 1:06 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 14/12/2022 19:35, Alexander H Duyck wrote:
> > On Tue, 2022-12-13 at 09:12 -0500, Aleksandr Burakov wrote:
> >> Index of info->se_info.atr can be overflow due to unchecked increment
> >> in the loop "for". The patch checks the value of current array index
> >> and doesn't permit increment in case of the index is equal to
> >> ST_NCI_ESE_MAX_LENGTH - 1.
> >>
> >> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >>
> >> Fixes: ed06aeefdac3 ("nfc: st-nci: Rename st21nfcb to st-nci")
> >> Signed-off-by: Aleksandr Burakov <a.burakov@rosalinux.ru>
> >> ---
> >> drivers/nfc/st-nci/se.c | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
> >> index ec87dd21e054..ff8ac1784880 100644
> >> --- a/drivers/nfc/st-nci/se.c
> >> +++ b/drivers/nfc/st-nci/se.c
> >> @@ -119,10 +119,11 @@ static u8 st_nci_se_get_bwi(struct nci_dev *ndev)
> >> /* Bits 8 to 5 of the first TB for T=1 encode BWI from zero to nine */
> >> for (i = 1; i < ST_NCI_ESE_MAX_LENGTH; i++) {
> >> td = ST_NCI_ATR_GET_Y_FROM_TD(info->se_info.atr[i]);
> >> - if (ST_NCI_ATR_TA_PRESENT(td))
> >> + if (ST_NCI_ATR_TA_PRESENT(td) && i < ST_NCI_ESE_MAX_LENGTH - 1)
> >> i++;
> >> if (ST_NCI_ATR_TB_PRESENT(td)) {
> >> - i++;
> >> + if (i < ST_NCI_ESE_MAX_LENGTH - 1)
> >> + i++;
> >> return info->se_info.atr[i] >> 4;
> >> }
> >> }
> >
> > Rather than adding 2 checks you could do this all with one check.
> > Basically you would just need to replace:
> > if (ST_NCI_ATR_TB_PRESENT(td)) {
> > i++;
> >
> > with:
> > if (ST_NCI_ATR_TB_PRESENT(td) && ++i < ST_NCI_ESE_MAX_LENGTH)
> >
> > Basically it is fine to increment "i" as long as it isn't being used as
> > an index so just restricting the last access so that we don't
> > dereference using it as an index should be enough.
>
> These are different checks - TA and TB. By skipping TA, your code is not
> equivalent. Was it intended?
Sorry, I wasn't talking about combining the TA and TB checks. I was
talking about combining the TB check and the bounds check so that you
didn't return and se_info_atr for a value that may not have actually
aligned due to the fact you had overflowed. Specifically, is skipping
the i++ the correct response to going out of bounds? I'm wondering if
you should be returning the default instead in the case of overflow?
The TA check could be modified so that it checks for "++i =
ST_NCI_ESE_MAX_LENGTH" and if that is true break rather than continue
in the loop.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] nfc: st-nci: array index overflow in st_nci_se_get_bwi()
2022-12-19 15:41 ` Alexander Duyck
@ 2022-12-19 15:46 ` Krzysztof Kozlowski
0 siblings, 0 replies; 5+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-19 15:46 UTC (permalink / raw)
To: Alexander Duyck
Cc: Aleksandr Burakov, Christophe Ricard, Samuel Ortiz, netdev,
linux-kernel, lvc-project
On 19/12/2022 16:41, Alexander Duyck wrote:
> On Mon, Dec 19, 2022 at 1:06 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 14/12/2022 19:35, Alexander H Duyck wrote:
>>> On Tue, 2022-12-13 at 09:12 -0500, Aleksandr Burakov wrote:
>>>> Index of info->se_info.atr can be overflow due to unchecked increment
>>>> in the loop "for". The patch checks the value of current array index
>>>> and doesn't permit increment in case of the index is equal to
>>>> ST_NCI_ESE_MAX_LENGTH - 1.
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>>
>>>> Fixes: ed06aeefdac3 ("nfc: st-nci: Rename st21nfcb to st-nci")
>>>> Signed-off-by: Aleksandr Burakov <a.burakov@rosalinux.ru>
>>>> ---
>>>> drivers/nfc/st-nci/se.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
>>>> index ec87dd21e054..ff8ac1784880 100644
>>>> --- a/drivers/nfc/st-nci/se.c
>>>> +++ b/drivers/nfc/st-nci/se.c
>>>> @@ -119,10 +119,11 @@ static u8 st_nci_se_get_bwi(struct nci_dev *ndev)
>>>> /* Bits 8 to 5 of the first TB for T=1 encode BWI from zero to nine */
>>>> for (i = 1; i < ST_NCI_ESE_MAX_LENGTH; i++) {
>>>> td = ST_NCI_ATR_GET_Y_FROM_TD(info->se_info.atr[i]);
>>>> - if (ST_NCI_ATR_TA_PRESENT(td))
>>>> + if (ST_NCI_ATR_TA_PRESENT(td) && i < ST_NCI_ESE_MAX_LENGTH - 1)
>>>> i++;
>>>> if (ST_NCI_ATR_TB_PRESENT(td)) {
>>>> - i++;
>>>> + if (i < ST_NCI_ESE_MAX_LENGTH - 1)
>>>> + i++;
>>>> return info->se_info.atr[i] >> 4;
>>>> }
>>>> }
>>>
>>> Rather than adding 2 checks you could do this all with one check.
>>> Basically you would just need to replace:
>>> if (ST_NCI_ATR_TB_PRESENT(td)) {
>>> i++;
>>>
>>> with:
>>> if (ST_NCI_ATR_TB_PRESENT(td) && ++i < ST_NCI_ESE_MAX_LENGTH)
>>>
>>> Basically it is fine to increment "i" as long as it isn't being used as
>>> an index so just restricting the last access so that we don't
>>> dereference using it as an index should be enough.
>>
>> These are different checks - TA and TB. By skipping TA, your code is not
>> equivalent. Was it intended?
>
> Sorry, I wasn't talking about combining the TA and TB checks. I was
> talking about combining the TB check and the bounds check so that you
> didn't return and se_info_atr for a value that may not have actually
> aligned due to the fact you had overflowed. Specifically, is skipping
> the i++ the correct response to going out of bounds? I'm wondering if
> you should be returning the default instead in the case of overflow?
>
> The TA check could be modified so that it checks for "++i =
> ST_NCI_ESE_MAX_LENGTH" and if that is true break rather than continue
> in the loop.
Ah, right. From that point of view, the first check (TA) also does not
look correct or equivalent. If we reached end of
ST_NCI_ESE_MAX_LENGTH(), we should not check TB on that entry. I would
propose to end the loop at that stage.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-12-19 15:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-13 14:12 [PATCH] nfc: st-nci: array index overflow in st_nci_se_get_bwi() Aleksandr Burakov
2022-12-14 18:35 ` Alexander H Duyck
2022-12-19 9:06 ` Krzysztof Kozlowski
2022-12-19 15:41 ` Alexander Duyck
2022-12-19 15:46 ` Krzysztof Kozlowski
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.