netdev - Re: [PATCH v12 net-next 09/12] crypto: chtls

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]

Message-ID: <20180319175758.2bc15291@epycfail>
Date:   Mon, 19 Mar 2018 17:57:58 +0100
From:   Stefano Brivio <sbrivio@...hat.com>
To:     Atul Gupta <atul.gupta@...lsio.com>
Cc:     davem@...emloft.net, herbert@...dor.apana.org.au,
        davejwatson@...com, sd@...asysnail.net,
        linux-crypto@...r.kernel.org, netdev@...r.kernel.org,
        ganeshgr@...lsio.com
Subject: Re: [PATCH v12 net-next 09/12] crypto: chtls - Inline TLS record Tx

On Mon, 19 Mar 2018 19:25:42 +0530
Atul Gupta <atul.gupta@...lsio.com> wrote:

> +static bool is_tls_skb(struct chtls_sock *csk, const struct sk_buff *skb)
> +{
> +	return skb_ulp_tls_skb_flags(skb);
> +}

Do you need this function?

> +/* Copy Key to WR */
> +static void tls_copy_tx_key(struct sock *sk, struct sk_buff *skb)
> +{
> +	struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> +	struct ulptx_sc_memrd *sc_memrd;
> +	struct ulptx_idata *sc;
> +	struct chtls_hws *hws = &csk->tlshws;
> +	struct chtls_dev *cdev = csk->cdev;
> +	u32 immdlen;
> +	int kaddr;

Reverse christmas tree comments (that you already received for several
versions) ignored.

> [...]
>
> +/*
> + * Returns true if an sk_buff carries urgent data.
> + */
> +static bool skb_urgent(struct sk_buff *skb)
> +{
> +	return (ULP_SKB_CB(skb)->flags & ULPCB_FLAG_URG) != 0;
> +}

You silently ignored most of the comment I had about this already on
v10.

> [...]
>
> +static void tls_tx_data_wr(struct sock *sk, struct sk_buff *skb,
> +			   int dlen, int tls_immd, u32 credits,
> +			   int expn, int pdus)
> +{
> +	struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> +	struct chtls_hws *hws = &csk->tlshws;
> +	struct net_device *dev = csk->egress_dev;
> +	struct adapter *adap = netdev2adap(dev);
> +	struct fw_tlstx_data_wr *req_wr;
> +	struct cpl_tx_tls_sfo *req_cpl;
> +	int iv_imm = skb_ulp_tls_skb_iv(skb);
> +	struct tls_scmd *scmd = &hws->scmd;
> +	struct tls_scmd *updated_scmd;
> +	unsigned int wr_ulp_mode_force;
> +	unsigned char data_type;
> +	unsigned char *req;
> +	int len = dlen + expn;
> +	int immd_len;

Reverse christmas tree.

> +static int chtls_expansion_size(struct sock *sk, int data_len,
> +				int fullpdu,
> +				unsigned short *pducnt)
> +{
> +	struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> +	struct chtls_hws *hws = &csk->tlshws;
> +	struct tls_scmd *scmd = &hws->scmd;
> +	int hdrlen = TLS_HEADER_LENGTH;
> +	int expnsize, frcnt, fraglast, fragsize;
> +	int expppdu;

Same here.

> +
> +	do {
> +		fragsize = hws->mfs;
> +
> +		if (SCMD_CIPH_MODE_G(scmd->seqno_numivs) ==
> +			SCMD_CIPH_MODE_AES_GCM) {
> +			frcnt = (data_len / fragsize);
> +			expppdu = GCM_TAG_SIZE + AEAD_EXPLICIT_DATA_SIZE +
> +					hdrlen;
> +			expnsize =  frcnt * expppdu;
> +
> +			if (fullpdu) {
> +				*pducnt = data_len / (expppdu + fragsize);
> +
> +				if (*pducnt > 32)
> +					*pducnt = 32;
> +				else if (!*pducnt)
> +					*pducnt = 1;
> +				expnsize = (*pducnt) * expppdu;
> +					break;

Was this supposed to be conditional?

> +			}
> +			fraglast = data_len % fragsize;
> +			if (fraglast > 0) {
> +				frcnt += 1;
> +				expnsize += expppdu;
> +			}
> +			break;
> +		}
> +		pr_info("unsupported cipher\n");
> +		expnsize = 0;
> +	} while (0);

And why the "do { ... } while (0)" anyway? Copied from a macro?

This series is *still* full of this kind of stuff. Is it such a bizarre
request to ask you to go through your code line by line and check it
carefully? Why should others do it instead?

> +
> +	return expnsize;
> +}
> +
> +/* WR with IV, KEY and CPL SFO added */
> +static void make_tlstx_data_wr(struct sock *sk, struct sk_buff *skb,
> +			       int tls_tx_imm, int tls_len, u32 credits)
> +{
> +	struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> +	struct chtls_hws *hws = &csk->tlshws;
> +	int pdus = DIV_ROUND_UP(tls_len, hws->mfs);
> +	unsigned short pdus_per_ulp = 0;
> +	int expn_sz;

Reverse christmas tree comments ignored here.

> [...]
> +int chtls_push_frames(struct chtls_sock *csk, int comp)
> +{
> +	struct sock *sk = csk->sk;
> +	struct tcp_sock *tp = tcp_sk(sk);
> +	struct chtls_hws *hws = &csk->tlshws;
> +	struct sk_buff *skb;
> +	int total_size = 0;
> +	int wr_size = sizeof(struct fw_ofld_tx_data_wr);

And here.

> [...]
> +int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> +{
> +	struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> +	struct tcp_sock *tp = tcp_sk(sk);
> +	struct chtls_dev *cdev = csk->cdev;
> +	struct sk_buff *skb;
> +	int mss, flags, err;
> +	int copied = 0;
> +	int hdrlen = 0;
> +	int recordsz = 0;
> +	long timeo;

And here.

> [...]
> +int chtls_sendpage(struct sock *sk, struct page *page,
> +		   int offset, size_t size, int flags)
> +{
> +	struct tcp_sock *tp = tcp_sk(sk);
> +	struct chtls_sock *csk;
> +	int mss, err, copied = 0;
> +	long timeo;

And here.

-- 
Stefano