netdev - Re: [PATCH] nfc: st-nci: array index overflow in st_nci_se_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5841f9021baf856c26fb27ac1d75fc1e29d3e044.camel@gmail.com>
Date:   Wed, 14 Dec 2022 10:35:59 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Aleksandr Burakov <a.burakov@...alinux.ru>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Christophe Ricard <christophe.ricard@...il.com>,
        Samuel Ortiz <sameo@...ux.intel.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        lvc-project@...uxtesting.org
Subject: Re: [PATCH] nfc: st-nci: array index overflow in st_nci_se_get_bwi()

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@...alinux.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.