linux-kernel - Re: [lvc-project] [PATCH] nfc: pn533: Add poll mod list filling check

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20240805-feadcc5dfd3905e1110a5068-pchelkin@ispras.ru>
Date: Mon, 5 Aug 2024 11:47:12 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: Krzysztof Kozlowski <krzk@...nel.org>, 
	Aleksandr Mishin <amishin@...rgos.ru>
Cc: Samuel Ortiz <sameo@...ux.intel.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org
Subject: Re: [lvc-project] [PATCH] nfc: pn533: Add poll mod list filling check

On Thu, 04. Jul 14:07, Krzysztof Kozlowski wrote:
> On 03/07/2024 09:26, Aleksandr Mishin wrote:
> > 
> > 
> > On 03.07.2024 8:02, Krzysztof Kozlowski wrote:
> >> On 02/07/2024 11:39, Aleksandr Mishin wrote:
> >>> In case of im_protocols value is 1 and tm_protocols value is 0 this
> >>
> >> Which im protocol has value 1 in the mask?
> >>
> >> The pn533_poll_create_mod_list() handles all possible masks, so your
> >> case is just not possible to happen.
> > 
> > Exactly. pn533_poll_create_mod_list() handles all possible specified 
> > masks. No im protocol has value 1 in the mask. In case of 'im_protocol' 
> 
> Which cannot happen.
> 
> > parameter has value of 1, no mod will be added. So dev->poll_mod_count 
> > will remain 0.
> 
> Which cannot happen.
> 
> > I assume 'im_protocol' parameter is "external" to this driver, it comes 
> > from outside and can contain any value, so driver has to be able to 
> > protect itself from incorrect values.
> 
> Did you read what I wrote? It cannot happen.

An important thing which unfortunately wasn't mentioned in commit log is
that these protocol values actually come from userspace via Netlink
interface (NFC_CMD_START_POLL operation). So a broken or malicious program
may pass a message containing a "bad" combination of protocol parameter
values so that dev->poll_mod_count is not incremented inside
pn533_poll_create_mod_list(), thus leading to division by zero.

nfc_genl_start_poll()
  nfc_start_poll()
    ->start_poll()
    pn533_start_poll()

Looking at pn533_poll_create_mod_list() source code, seems there may be a
number of such "bad" combinations: e.g. when passed tm_protocols is 0 and
im_protocols is 1.

CAP_NET_ADMIN is currently required to perform device control operations
but it's not a point for the situation to be neglected.

Regarding the patch, maybe it'd be better to include the check inside
pn533_poll_create_mod_list() itself and return an error? That'd be more
convenient if someday this function would be called elsewhere in the code
and dev->poll_mod_count must still be guaranteed to be incremented at
least once.