linux-kernel - Re: [PATCH 2/2] net/9p: fix response size check in p9_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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 22 Nov 2022 09:21:43 +0900
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Christian Schoenebeck <linux_oss@...debyte.com>
Cc:     Stefano Stabellini <sstabellini@...nel.org>,
        v9fs-developer@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
        GUO Zihua <guozihua@...wei.com>
Subject: Re: [PATCH 2/2] net/9p: fix response size check in p9_check_errors()

Christian Schoenebeck wrote on Tue, Nov 22, 2022 at 12:04:08AM +0100:
> Since 60ece0833b6c (net/9p: allocate appropriate reduced message buffers)
> it is no longer appropriate to check server's response size against
> msize. Check against the previously allocated buffer capacity instead.

Thanks for the follow up!

>  - Omit this size check entirely for zero-copy messages, as those always
>    allocate 4k (P9_ZC_HDR_SZ) linear buffers which are not used for actual
>    payload and can be much bigger than 4k.

[review includes the new flag patch]

hmm, unless there's anywhere else you think we might use these flags it
looks simpler to just pass a flag to p9_check_errors?

In particular adding a bool in this position is not particularly efficient:
-------(pahole)-----
struct p9_fcall {
	u32                        size;                 /*     0     4 */
	u8                         id;                   /*     4     1 */

	/* XXX 1 byte hole, try to pack */

	u16                        tag;                  /*     6     2 */
	size_t                     offset;               /*     8     8 */
	size_t                     capacity;             /*    16     8 */
	bool                       zc;                   /*    24     1 */

	/* XXX 7 bytes hole, try to pack */

	struct kmem_cache *        cache;                /*    32     8 */
	u8 *                       sdata;                /*    40     8 */

	/* size: 48, cachelines: 1, members: 8 */
	/* sum members: 40, holes: 2, sum holes: 8 */
	/* last cacheline: 48 bytes */
};
----------------
Not that adding it between id and tag sounds better to me, so this is
probably just as good as anywhere else :-D

Anyway, I'm just nitpicking -- on principle I agree just whitelisting zc
requests from this check makes most sense, happy with either way if you
think this is better for the future.

>  - Replace p9_debug() by pr_err() to make sure this message is always
>    printed in case this error is triggered.
> 
>  - Add 9p message type to error message to ease investigation.

Yes to these log changes!

> 
> Signed-off-by: Christian Schoenebeck <linux_oss@...debyte.com>
> ---
>  net/9p/client.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 30dd82f49b28..63f13dd1ecff 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -514,10 +514,10 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
>  	int ecode;
>  
>  	err = p9_parse_header(&req->rc, NULL, &type, NULL, 0);
> -	if (req->rc.size >= c->msize) {
> -		p9_debug(P9_DEBUG_ERROR,
> -			 "requested packet size too big: %d\n",
> -			 req->rc.size);
> +	if (req->rc.size > req->rc.capacity && !req->rc.zc) {
> +		pr_err(
> +			 "requested packet size too big: %d does not fit %ld (type=%d)\n",
> +			 req->rc.size, req->rc.capacity, req->rc.id);

Haven't seen this style before -- is that what qemu uses?
We normally keep the message on first line and align e.g.
> +             pr_err("requested packet size too big: %d does not fit %ld (type=%d)\n",
> +                    req->rc.size, req->rc.capacity, req->rc.id);

(at least what's what other grep -A 1 'pr_err.*,$' seem to do, and
checkpatch is happier with that)

-- 
Dominique