netdev - Re: [PATCH iproute2] libnetlink.c, ss.c: properly handle fread() error

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]

Date:   Tue, 29 Oct 2019 12:35:50 +0100
From:   michal.lyszczek@...c.pl
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH iproute2] libnetlink.c, ss.c: properly handle fread()
 error

Hello Stephen,
On 2019-10-28 21:21:28, Stephen Hemminger wrote:
> On Thu, 24 Oct 2019 23:20:01 +0200
> Michał Łyszczek <michal.lyszczek@...c.pl> wrote:
>
> > fread(3) returns size_t data type which is unsigned, thus check
> > `if (fread(...) < 0)' is always false. To check if fread(3) has
> > failed, user should check if return is 0 and then check error
> > indicator with ferror(3).
>
> You did find something that probably has been broken for a long time.
>
> First off, not sure why libnetlink is using fread here anyway.
> It adds another copy to all I/O which can matter with 1M routes.

I don't this is a problem. Of course, this could be optimized with read(2)
but these functions are (or at least I think they are) called very rarely.
Optimal solution with read(2) will surely be much more complex than using
fread(3). I'm not sure if minor performance gain is worth bigger complexity.

> Also the man page for fread() implies that truncated reads (not
> just zero) can happen on error. Better to check that full read was
> completed or at least a valid netlink header?

Yes, you are right, I must have missed that. I've changed patch to
take this into account. I think, since this code parses precise binary
data, each call to fread(3) should return exact ammount of bytes as
what was requested as reading less then expected could lead to corrupt
read later anyway.

For example if `l = 3' and `NLMSG_ALIGN(l) == 4' doing

    status = fread(NLMSG_DATA(h), 1, NLMSG_ALIGN(l), rtnl);
    if (status < l)
        error;

Will result in error when fread(3) returns 3 bytes (and error), as
this will move stream pointer by 3 bytes instead of 4, and next
call to fread(3) will first read last DATA byte and then header
bytes, which will result in corrupted header and possible misleading
error later in execution - I belive errors should be reported as
soon as possible.

Please review newly attached patch (in another mail).

-- 
.-----------------.-------------------.---------------------.------------------.
| Michal Lyszczek | Embedded C, Linux |   Company Address   |  .-. open source |
| +48 727 564 419 | Software Engineer | Leszczynskiego 4/29 |  oo|  supporter  |
| https://bofc.pl `----.--------------: 50-078 Wroclaw, Pol | /`'\      &      |
| GPG FF1EBFE7E3A974B1 | Bits of Code | NIP:  813 349 58 78 |(\_;/) programer  |
`----------------------^--------------^---------------------^------------------'

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)