linux-kernel - Re: [PATCH 5/7] staging: usbip: Add encryption support to kernel

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]

Date:	Thu, 26 Sep 2013 14:48:10 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Dominik Paulus <dominik@...aulus.de>
Cc:	Anthony Foiani <anthony.foiani@...il.com>,
	devel@...verdev.osuosl.org, linux-usb@...r.kernel.org,
	linux-kernel@...cs.fau.de,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	usbip-devel@...ts.sourceforge.net,
	Kurt Kanzenbach <ly80toro@....cs.fau.de>,
	Tobias Polzer <tobias.polzer@....de>,
	Harvey Yang <harvey.huawei.yang@...il.com>,
	linux-kernel@...r.kernel.org, Joe Perches <joe@...ches.com>,
	Bart Westgeest <bart@...rys.com>,
	Dominik Paulus <dominik.paulus@....de>,
	Ilija Hadzic <ihadzic@...earch.bell-labs.com>,
	Jake Champlin <jake.champlin.27@...il.com>,
	Stefan Reif <ke42caxa@....cs.fau.de>,
	Bernard Blackham <b-linuxgit@...gestprime.net>
Subject: Re: [PATCH 5/7] staging: usbip: Add encryption support to kernel

On Thu, Sep 26, 2013 at 12:18:34PM +0200, Dominik Paulus wrote:
> > I think a return of zero should mean total = -EBADMSG;.  In other words
> > this check should be "if (ret < 0) {" and we hit the next else if.
> > Same below again.
> 
> As we are wrapping kernel_recvmsg here, we wanted to leave the semantics
> intact as far as possible. The calling code already checks for the correct
> size.

Hm...  Ok.  Sometimes zero is interpretted as a connection closed and
sometimes reading less than expected is considered a TCP error.

> No, currently, the caller (usbip_sendmsg() / usbip_recvmsg() are the
> only functions calling usbip_crypt(), which itself is static) ensures
> this.
> Admittedly, this isn't great design. We added a check for packetsize <
> USBIP_AUTHSIZE and an appropiate return here.
> 
> > > +	if (encrypt)
> > > +		ret = crypto_aead_encrypt(req);
> > > +	else
> > > +		ret = crypto_aead_decrypt(req);
> > > +
> > 
> > Good on you for figuring out what crypto_aead_en/decrypt() returns.
> > Where are these functions documented?
> > 
> > > +        switch (ret) {
> > > +        case 0: /* Success */
> > > +                break;
> > > +        case -EINPROGRESS:
> > > +        case -EBUSY:
> > > +                wait_for_completion(&result.completion);
> > > +                break;
> > > +        default:
> > > +                aead_request_free(req);
> > > +                return ret;
> > > +        }
> > > +
> 
> They aren't, actually. Documentation/crypto/api-intro.txt refers to the
> regression test module, which uses exactly those return-values in
> crypto/testmgr.c.

Well that sucks.

> We noticed that wait_for_completion might not be the best idea, since it could
> hang indefinitely, testmgr.c uses wait_for_completion_interruptible. Do we
> want 'interruptible' or 'killable' here?

I think you want the interruptible one wait_for_completion_interruptible()

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/