linux-kernel - Re: [PATCH v9 1/1] Add support for IPMB driver

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: <20190603201325.GC2383@kunai>
Date:   Mon, 3 Jun 2019 22:13:26 +0200
From:   Wolfram Sang <wsa@...-dreams.de>
To:     Asmaa Mnebhi <Asmaa@...lanox.com>
Cc:     "minyard@....org" <minyard@....org>,
        Vadim Pasternak <vadimp@...lanox.com>,
        Michael Shych <michaelsh@...lanox.com>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: Re: [PATCH v9 1/1] Add support for IPMB driver

Hi Asmaa,

sorry for the long wait. I missed this mail was still sitting in my
Drafts folder :(

> >> Am I overlooking something? Why are you protecting an atomic_read with a spinlock?
> 
> A thread would lock the ipmb_dev->lock spinlock (above) for all the code below ONLY IF the atomic_read for the request_queue_len reports a value different from 0:

Well, not really. The spinlock is taken _before_ the atomic read. But
the read is atomic, so there should be no need. I am asking if the code
could look like this?

+	while (!atomic_read(&ipmb_dev->request_queue_len)) {
+		if (non_blocking)
+			return -EAGAIN;
+
+		res = wait_event_interruptible(ipmb_dev->wait_queue,
+				atomic_read(&ipmb_dev->request_queue_len));
+		if (res)
+			return res;
+	}
+
+	spin_lock_irqsave(&ipmb_dev->lock, flags);
+	if (list_empty(&ipmb_dev->request_queue)) {

> if (list_empty(&ipmb_dev->request_queue)) {
> 260 +               dev_err(&ipmb_dev->client->dev, "request_queue is empty\n");
> 261 +               spin_unlock_irqrestore(&ipmb_dev->lock, flags);

The unlock operation could come before the dev_err. We don't need to
protect the printout and save time with the spinlock held.

> > +	rq_sa = msg[RQ_SA_8BIT_IDX] >> 1;
> > +	netf_rq_lun = msg[NETFN_LUN_IDX];
> > +	/*
> > +	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
> > +	 * i2c_smbus_write_block_data_local
> > +	 */
> > +	msg_len = msg[IPMB_MSG_LEN_IDX] - SMBUS_MSG_HEADER_LENGTH;
> > +
> > +	strcpy(rq_client.name, "ipmb_requester");
> > +	rq_client.adapter = ipmb_dev->client->adapter;
> > +	rq_client.flags = ipmb_dev->client->flags;
> > +	rq_client.addr = rq_sa;
> 
> >> Is it possible to determine in a race-free way if rq_sa (which came
> >> from userspace AFAIU) is really the address from which the request
> >> came in (again if I understood all this correctly)?
> Yes there is. I see 2 options:
> 
> 1) This is less explicit than option 2 but uses existing code and is
> simpler. we can use the ipmb_verify_checksum1 function since the IPMB
> response format is as follows:
> Byte 1: rq_sa
> Byte 2: netfunction/rqLUN
> Byte 3: checksum1

Hmmm, does that really prove that rq_sa is the same address the request
came from? Or does it only prove that the response packet is not
mangled?

> So if checksum1 is verified, it means rq_sa is correct.
> 
> 2) I am not sure we want this but have a global variable which stores
> the address of the requester once the first request is received. We
> would compare that address with the one received from userspace in the
> code above.

Can there be only one requester in the system?

Thanks,

   Wolfram

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