On 31.03.2021 18:26, Dan Carpenter wrote: > Hi Alexander, > > I enjoyed reading your blog entries for CVE-2017-2636 and CVE-2019-18683. Hello Dan, thanks! > https://lore.kernel.org/lkml/20210201084719.2257066-1-alex.popov@xxxxxxxxx/ > > My understanding is that you were able to write a privilege escalataion > bug for this? Are you going to do a write up for this one as well? Yes, I will give a talk about my exploit for CVE-2021-26708 at Zer0Con and publish the article next week: https://zer0con.org/#speaker-section I'm finishing the article right now. > Was the trick to exploiting it to use the free in vsock_deassign_transport()? Yes! > I've wanted to look for these in Smatch (my static analysis tool) for a > long time. I wrote a very naive first draft implementation. What it > looks for is when we access struct sock members before calling > lock_sock(). It generated 8 warnings. Six were false positives but I > think that two were probably real bugs. Cool! > net/nfc/llcp_sock.c:313 nfc_llcp_getsockopt() warn: unlocked access 'llcp_sock->local' expected lock '&sk->sk_lock.slock' > net/nfc/llcp_sock.c:597 llcp_sock_release() warn: unlocked access 'llcp_sock->local' expected lock '&sk->sk_lock.slock' > > But I think it would be hard to exploit those because the race is very > tiny. I can't say about these two cases, but for CVE-2021-26708 the race window is huge. To hit the race condition I have two pthreads that work in parallel: - 1st pthread calls lock_sock() and performs some work with the virtual socket; - 2nd pthread saves the value to a local variable and hangs on lock_sock(), since the lock is already acquired. When the first pthread finishes the work and releases the lock, the second pthread is able to finish lock_sock() and proceed with the out-of-date value in the local variable. Maybe this strategy can be used in the cases that you detected. > Several people (maybe most recently Lukas Bulwahn but someone else at > Linux Plumbers) suggested that a way to find race conditions is to look > at the line directly after taking a lock. So if you have: > > mutex_lock(&dsp->pwr_lock); > if (!dsp->wmfw_file_name || !dsp->booted) > > Then that means that the ->pwr_lock is protecting ->wmfw_file_name and > ->booted. So I wrote a check that made a list of these sorts of > pairings. Then I wrote a check that said: > > If you take save a "protected" struct member and then take the lock > print a warning that the access should have been inside the lock. So a > warning looks like this: > > sound/pci/cs46xx/cs46xx_lib.c:2148 snd_cs46xx_spdif_default_get() warn: unlocked access 'ins' (line 2146) expected lock '&chip->spos_mutex' > 2142 static int snd_cs46xx_spdif_default_get(struct snd_kcontrol *kcontrol, > 2143 struct snd_ctl_elem_value *ucontrol) > 2144 { > 2145 struct snd_cs46xx *chip = snd_kcontrol_chip(kcontrol); > 2146 struct dsp_spos_instance * ins = chip->dsp_spos_instance; > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Smatch is saying this assignment should have been done > > 2147 > 2148 mutex_lock(&chip->spos_mutex); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > inside this lock. > > 2149 ucontrol->value.iec958.status[0] = _wrap_all_bits((ins->spdif_csuv_default >> 24) & 0xff); > 2150 ucontrol->value.iec958.status[1] = _wrap_all_bits((ins->spdif_csuv_default >> 16) & 0xff); > 2151 ucontrol->value.iec958.status[2] = 0; > 2152 ucontrol->value.iec958.status[3] = _wrap_all_bits((ins->spdif_csuv_default) & 0xff); > 2153 mutex_unlock(&chip->spos_mutex); > 2154 > 2155 return 0; > 2156 } > > That seems probably true but also a pretty harmless. I've attached a > sample of the output just in case you're curious. > > Do you have any idea how I could get more worthwhile results than this? I would also propose to use your approach and search for access to "protected" struct fields after UNlock functions. Best regards, Alexander
