linux-kernel - Re: [PATCH] 9p/net: fix improper handling of bogus negative read/write replies

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] [day] [month] [year] [list]

Message-ID: <Z9dEiQUyDtf7nx0e@codewreck.org>
Date: Mon, 17 Mar 2025 06:37:13 +0900
From: Dominique Martinet <asmadeus@...ewreck.org>
To: Christian Schoenebeck <linux_oss@...debyte.com>
Cc: Eric Van Hensbergen <ericvh@...nel.org>,
	Latchesar Ionkov <lucho@...kov.net>, v9fs@...ts.linux.dev,
	linux-kernel@...r.kernel.org, Robert Morris <rtm@....edu>
Subject: Re: [PATCH] 9p/net: fix improper handling of bogus negative
 read/write replies

Christian Schoenebeck wrote on Sun, Dec 22, 2024 at 04:29:58PM +0100:
> > @@ -1592,11 +1593,11 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
> >  		return 0;
> >  	}
> >  	if (rsize < received) {
> > -		pr_err("bogus RREAD count (%d > %d)\n", received, rsize);
> > +		pr_err("bogus RREAD count (%u > %u)\n", received, rsize);
> >  		received = rsize;
> 
> Does `received = rsize` make sense here? I would rather do `received = 0` to
> prevent copying garbage below, that would be ignored by caller on error case
> anyway.

Good point.. We're not setting *err here so this isn't an error (and
thus not ignored), so I guess it sort of makes sense if you look at it
from a "best effort" point of view, but I agree a bogus reply probably
better return an error.

OTOH it's a change of behaviour so I'd rather have it in another patch,
will send separately.

> > @@ -1623,9 +1624,9 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
> >  	*err = 0;
> >  
> >  	while (iov_iter_count(from)) {
> > -		int count = iov_iter_count(from);
> > -		int rsize = fid->iounit;
> > -		int written;
> > +		size_t count = iov_iter_count(from);
> > +		size_t rsize = fid->iounit;
> 
> I think that would break 64-bit big-endian systems, as `rsize` is passed via
> format below as "d" (32-bit) type.

Good catch, thanks!
-- 
Dominique Martinet | Asmadeus