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

[Fedor Pchelkin]

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.