[PATCH v4 0/7] char: xillybus: Harden driver and improve code quality

[PATCH v4 0/7] char: xillybus: Harden driver and improve code quality

[Eli Billauer]

This patch set consists of several boundary and sanity checks, gaining
better control of execution flow and fixing minor coding issues. No
difference is expected in the driver's behavior under normal conditions,
except for one change that might improve bandwidth performance
marginally.

Why all of the sudden? These patches are the result of a comprehensive
AI-assisted code review, using Deepseek, Kimi Thinking K2.6, ChatGPT,
and Claude Sonnet 4.6 as plain prompt tools.

I've spent three weeks in what I call a "reverse rubber duck" session,
asking these LLMs to find bugs in the code and following up on different
leads, the vast majority of which turned out to be no more than quack.
Occasionally the points made by AI were valid, and in others, the mere
effort to explain why the remark is wrong to the rubber duck led to
finding real issues.

Almost none of the fixes suggested by AI were even near the actual
corrections in this patch set.

The v1 iteration of this series was rejected due to an error in the
error path. Specifically, the @rc variable was not assigned a value
before a goto statement following a failed allocation. This oversight
was caught during compilation with LLVM=1, but was missed by GCC and the
AI tools used during the review (and myself of course).

All patches are unchanged since v1, except for "Add defensive sanity
checks", which fixes the said error and also makes a couple of minor
adjustments.

The change from v2 to v3 merely consists of adding Assisted-by tags
to the patches as required.

This v4 submission is the response to some of Sashiko's remarks on the
v3 submission:

https://sashiko.dev/#/patchset/20260724094302.50761-1-eli.billauer@gmail.com

Three remarks relate to the patches in these patch set, and have been
corrected accordingly, with Assisted-by tags duly added. Sashiko also
had several other valid remarks which call for additional patches.
These will be submitted once the processing of this patch set is done.

Thanks & regards,
   Eli

Eli Billauer (7):
  char: xillybus: Improve control of execution flow with mutexes
  char: xillybus: Remove duplicate error path code
  char: xillybus: Avoid possible bandwidth inefficiency
  char: xillybus: Use unsigned arithmetic for jiffies differences
  char: xillybus: Integer arithmetic improvements
  char: xillybus: Add defensive sanity checks
  char: xillybus: Ignore and report unsolicited interrupts

 drivers/char/xillybus/xillybus.h       |  3 +
 drivers/char/xillybus/xillybus_class.c | 18 ++++-
 drivers/char/xillybus/xillybus_class.h |  3 +
 drivers/char/xillybus/xillybus_core.c  | 95 +++++++++++++++++++++++---
 drivers/char/xillybus/xillybus_of.c    |  3 +
 drivers/char/xillybus/xillyusb.c       | 65 +++++++++++++-----
 6 files changed, 158 insertions(+), 29 deletions(-)

-- 
2.34.1

[PATCH v4 1/7] char: xillybus: Improve control of execution flow with mutexes

[Eli Billauer]

This commit addresses two issues by using mutexes:

(1) Add a mutex to protect the fifo_buf_order global variable. The
    purpose of this variable is avoid repeated failed calls to
    __get_free_pages() for allocating FIFO memory, when the chunk
    size was too big. However, if two drivers are initialized at the
    same time, fifo_init() may run in parallel, and fifo_buf_order
    may be reduced too much. This is a far-fetched scenario, now
    completely prevented by fifo_buf_order_mutex.

(2) setup_channels() acquires process_in_mutex to prevent
    process_bulk_in() from accessing the xillyusb_dev struct. With
    correctly working hardware, process_bulk_in() is never called while
    setup_channels() runs, because the device has no reason to send data
    in that phase. The mutex ensures that process_bulk_in() does not
    touch the members that setup_channels() alters.

    There is no similar protection for data flow in the other direction,
    because during the setup process, the only outbound data is the
    BULK endpoint used for commands, and it remains untouched after its
    initial setup.

Assisted-by: Deepseek:v4-pro Kimi:K2.6 ChatGPT:GPT-5.5 Claude:Sonnet-4.6
Assisted-by: Sashiko-0.2.5:gemini-3.1-pro-preview
Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---

Notes:
    Changelog:
    =========
    
    Changes v3->v4:
     -- Use plain mutex_lock() in fifo_init() rather than guard() in order
        to avoid mixing guard() with goto, following Sashiko's remark + add
        attribution to Sashiko for this.
    
    Changes v2->v3:
     -- Add Assisted-by tag to description
    
    No change on v1->v2.

 drivers/char/xillybus/xillyusb.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index 34e7ad3bcab3..560c3568fc96 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -50,6 +50,7 @@ MODULE_LICENSE("GPL v2");
 static const char xillyname[] = "xillyusb";
 
 static unsigned int fifo_buf_order;
+static DEFINE_MUTEX(fifo_buf_order_mutex);
 static struct workqueue_struct *wakeup_wq;
 
 #define USB_VENDOR_ID_XILINX		0x03fd
@@ -375,6 +376,8 @@ static int fifo_init(struct xillyfifo *fifo,
 
 	unsigned int log2_fifo_buf_size;
 
+	mutex_lock(&fifo_buf_order_mutex);
+
 retry:
 	log2_fifo_buf_size = fifo_buf_order + PAGE_SHIFT;
 
@@ -395,8 +398,10 @@ static int fifo_init(struct xillyfifo *fifo,
 
 	fifo->mem = kmalloc_array(fifo->bufnum, sizeof(void *), GFP_KERNEL);
 
-	if (!fifo->mem)
+	if (!fifo->mem) {
+		mutex_unlock(&fifo_buf_order_mutex);
 		return -ENOMEM;
+	}
 
 	for (i = 0; i < fifo->bufnum; i++) {
 		fifo->mem[i] = (void *)
@@ -413,6 +418,8 @@ static int fifo_init(struct xillyfifo *fifo,
 	fifo->writebuf = 0;
 	spin_lock_init(&fifo->lock);
 	init_waitqueue_head(&fifo->waitq);
+
+	mutex_unlock(&fifo_buf_order_mutex);
 	return 0;
 
 memfail:
@@ -426,6 +433,7 @@ static int fifo_init(struct xillyfifo *fifo,
 		fifo_buf_order--;
 		goto retry;
 	} else {
+		mutex_unlock(&fifo_buf_order_mutex);
 		return -ENOMEM;
 	}
 }
@@ -1943,6 +1951,9 @@ static int setup_channels(struct xillyusb_dev *xdev,
 	struct xillyusb_channel *chan, *new_channels;
 	int i;
 
+	/* Don't let process_bulk_in() run while we change the channels */
+	guard(mutex)(&xdev->process_in_mutex);
+
 	chan = kzalloc_objs(*chan, num_channels);
 	if (!chan)
 		return -ENOMEM;
-- 
2.34.1

[PATCH v4 2/7] char: xillybus: Remove duplicate error path code

[Eli Billauer]

Remove dedicated code for the error path, which is both redundant and
incorrect: It calls kref_put() before mutex_unlock(&chan->lock). As
kref_put() may result in releasing the memory containing the mutex, this
could lead to UAF.

Assisted-by: Deepseek:v4-pro Kimi:K2.6 ChatGPT:GPT-5.5 Claude:Sonnet-4.6
Assisted-by: Sashiko-0.2.5:gemini-3.1-pro-preview
Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---

Notes:
    Changelog:
    =========
    
    Changes v3->v4:
      -- Correct the commit description to indicate that the removed duplicate
         code wasn't only redundant, but could also lead to a UAF, as pointed
         out by Sashiko, and an attribution is hence added too.
    
    Changes v2->v3:
     -- Add Assisted-by tag to description
    
    No change on v1->v2.

 drivers/char/xillybus/xillyusb.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index 560c3568fc96..ee819e2e3f82 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -1424,16 +1424,12 @@ static int xillyusb_open(struct inode *inode, struct file *filp)
 	if (filp->f_mode & FMODE_WRITE)
 		chan->open_for_write = 0;
 
+unmutex_fail:
 	mutex_unlock(&chan->lock);
 
 	kref_put(&xdev->kref, cleanup_dev);
 
 	return rc;
-
-unmutex_fail:
-	kref_put(&xdev->kref, cleanup_dev);
-	mutex_unlock(&chan->lock);
-	return rc;
 }
 
 static ssize_t xillyusb_read(struct file *filp, char __user *userbuf,
-- 
2.34.1

[PATCH v4 3/7] char: xillybus: Avoid possible bandwidth inefficiency

[Eli Billauer]

The host flow controls the payload data flow from the FPGA by sending
OPCODE_SET_CHECKPOINT messages. Fix the condition for sending such a
message, to correctly handle the case where leap < 0.

The previous expression leap > (fifo->size >> 3) was not intended to
evaluate true when leap is negative. However, due to C's integer
promotion rules, leap (of s32 type) is promoted to unsigned int when
compared with the unsigned fifo->size >> 3 expression. As a result,
negative leap values are interpreted as large positive numbers, causing
the condition to evaluate true unintentionally.

Consequently, the device receives correctly formed checkpoint messages
that encourage it to send data, but too frequently. This may cause the
device to send short data chunks, wasting USB bandwidth.

Assisted-by: Deepseek:v4-pro Kimi:K2.6 ChatGPT:GPT-5.5 Claude:Sonnet-4.6
Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---

Notes:
    Changelog:
    =========
    
    No change on v3->v4.
    
    Changes v2->v3:
     -- Add Assisted-by tag to description
    
    No change on v1->v2.

 drivers/char/xillybus/xillyusb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index ee819e2e3f82..ef5b1816b277 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -1512,8 +1512,8 @@ static ssize_t xillyusb_read(struct file *filp, char __user *userbuf,
 		 */
 
 		if (chan->read_data_ok &&
-		    (leap > (fifo->size >> 3) ||
-		     (checkpoint_for_complete && leap > 0))) {
+		    (leap > 0 && (leap > (fifo->size >> 3) ||
+				  checkpoint_for_complete))) {
 			chan->in_current_checkpoint = checkpoint;
 			rc = xillyusb_send_opcode(xdev, chan_num,
 						  OPCODE_SET_CHECKPOINT,
-- 
2.34.1

[PATCH v4 4/7] char: xillybus: Use unsigned arithmetic for jiffies differences

[Eli Billauer]

Change the type of jiffies-related deadline variables from long to
unsigned long, and remove unnecessary casts when computing time
remaining as deadline - jiffies.

No functional change is expected: although signed overflow is undefined
in the C standard, processors perform the calculation correctly in
practice. Using unsigned arithmetic is nevertheless the proper way to
handle jiffies differences.

Assisted-by: Deepseek:v4-pro Kimi:K2.6 ChatGPT:GPT-5.5 Claude:Sonnet-4.6
Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---

Notes:
    Changelog:
    =========
    
    No change on v3->v4.
    
    Changes v2->v3:
     -- Add Assisted-by tag to description
    
    No change on v1->v2.

 drivers/char/xillybus/xillybus_core.c |  5 +++--
 drivers/char/xillybus/xillyusb.c      | 17 ++++++++++-------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/char/xillybus/xillybus_core.c b/drivers/char/xillybus/xillybus_core.c
index 952ef149aba1..7acebc1e6050 100644
--- a/drivers/char/xillybus/xillybus_core.c
+++ b/drivers/char/xillybus/xillybus_core.c
@@ -694,7 +694,8 @@ static ssize_t xillybus_read(struct file *filp, char __user *userbuf,
 	unsigned long flags;
 	int bytes_done = 0;
 	int no_time_left = 0;
-	long deadline, left_to_sleep;
+	unsigned long deadline;
+	long left_to_sleep;
 	struct xilly_channel *channel = filp->private_data;
 
 	int empty, reached_eof, exhausted, ready;
@@ -938,7 +939,7 @@ static ssize_t xillybus_read(struct file *filp, char __user *userbuf,
 			return -EINTR;
 		}
 
-		left_to_sleep = deadline - ((long) jiffies);
+		left_to_sleep = deadline - jiffies;
 
 		/*
 		 * If our time is out, skip the waiting. We may miss wr_sleepy
diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index ef5b1816b277..e2270a64b659 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -1132,12 +1132,13 @@ static int xillyusb_send_opcode(struct xillyusb_dev *xdev,
  */
 
 static int flush_downstream(struct xillyusb_channel *chan,
-			    long timeout,
+			    unsigned long timeout,
 			    bool interruptible)
 {
 	struct xillyusb_dev *xdev = chan->xdev;
 	int chan_num = chan->chan_idx << 1;
-	long deadline, left_to_sleep;
+	unsigned long deadline;
+	long left_to_sleep;
 	int rc;
 
 	if (chan->flushed)
@@ -1146,7 +1147,8 @@ static int flush_downstream(struct xillyusb_channel *chan,
 	deadline = jiffies + 1 + timeout;
 
 	if (chan->flushing) {
-		long cancel_deadline = jiffies + 1 + XILLY_RESPONSE_TIMEOUT;
+		unsigned long cancel_deadline =
+			jiffies + 1 + XILLY_RESPONSE_TIMEOUT;
 
 		chan->canceled = 0;
 		rc = xillyusb_send_opcode(xdev, chan_num,
@@ -1157,7 +1159,7 @@ static int flush_downstream(struct xillyusb_channel *chan,
 
 		/* Ignoring interrupts. Cancellation must be handled */
 		while (!chan->canceled) {
-			left_to_sleep = cancel_deadline - ((long)jiffies);
+			left_to_sleep = cancel_deadline - jiffies;
 
 			if (left_to_sleep <= 0) {
 				report_io_error(xdev, -EIO);
@@ -1207,7 +1209,7 @@ static int flush_downstream(struct xillyusb_channel *chan,
 	}
 
 	while (chan->flushing) {
-		left_to_sleep = deadline - ((long)jiffies);
+		left_to_sleep = deadline - jiffies;
 
 		if (left_to_sleep <= 0)
 			return -ETIMEDOUT;
@@ -1440,7 +1442,8 @@ static ssize_t xillyusb_read(struct file *filp, char __user *userbuf,
 	struct xillyfifo *fifo = chan->in_fifo;
 	int chan_num = (chan->chan_idx << 1) | 1;
 
-	long deadline, left_to_sleep;
+	unsigned long deadline;
+	long left_to_sleep;
 	int bytes_done = 0;
 	bool sent_set_push = false;
 	int rc;
@@ -1469,7 +1472,7 @@ static ssize_t xillyusb_read(struct file *filp, char __user *userbuf,
 		bytes_done += rc;
 		chan->in_consumed_bytes += rc;
 
-		left_to_sleep = deadline - ((long)jiffies);
+		left_to_sleep = deadline - jiffies;
 
 		/*
 		 * Some 32-bit arithmetic that may wrap. Note that
-- 
2.34.1

[PATCH v4 5/7] char: xillybus: Integer arithmetic improvements

[Eli Billauer]

Choose unsigned integers instead of signed where natural, and also ensure
overflow wraps as expected.

Simplify an arithmetic expression too.

No functional change is expected, as the relevant variables normally never
reach values where this transition matters.

Assisted-by: Deepseek:v4-pro Kimi:K2.6 ChatGPT:GPT-5.5 Claude:Sonnet-4.6
Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---

Notes:
    Changelog:
    =========
    
    No change on v3->v4.
    
    Changes v2->v3:
     -- Add Assisted-by tag to description
    
    No change on v1->v2.

 drivers/char/xillybus/xillybus_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/char/xillybus/xillybus_core.c b/drivers/char/xillybus/xillybus_core.c
index 7acebc1e6050..b264578b2572 100644
--- a/drivers/char/xillybus/xillybus_core.c
+++ b/drivers/char/xillybus/xillybus_core.c
@@ -343,7 +343,8 @@ static int xilly_map_single(struct xilly_endpoint *ep,
 static int xilly_get_dma_buffers(struct xilly_endpoint *ep,
 				 struct xilly_alloc_state *s,
 				 struct xilly_buffer **buffers,
-				 int bufnum, int bytebufsize)
+				 unsigned int bufnum,
+				 unsigned int bytebufsize)
 {
 	int i, rc;
 	dma_addr_t dma_addr;
@@ -431,8 +432,8 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 	struct device *dev = ep->dev;
 	int i, entry, rc;
 	struct xilly_channel *channel;
-	int channelnum, bufnum, bufsize, format, is_writebuf;
-	int bytebufsize;
+	unsigned int channelnum, bufnum, bufsize, format, is_writebuf;
+	unsigned int bytebufsize;
 	int synchronous, allowpartial, exclusive_open, seekable;
 	int supports_nonempty;
 	int msg_buf_done = 0;
@@ -531,8 +532,7 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 			channel->log2_element_size = ((format > 2) ?
 						      2 : format);
 
-			bytebufsize = bufsize *
-				(1 << channel->log2_element_size);
+			bytebufsize = bufsize << channel->log2_element_size;
 
 			buffers = devm_kcalloc(dev, bufnum,
 					       sizeof(struct xilly_buffer *),
@@ -589,7 +589,7 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 static int xilly_scan_idt(struct xilly_endpoint *endpoint,
 			  struct xilly_idt_handle *idt_handle)
 {
-	int count = 0;
+	unsigned int count = 0;
 	unsigned char *idt = endpoint->channels[1]->wr_buffers[0]->addr;
 	unsigned char *end_of_idt = idt + endpoint->idtlen - 4;
 	unsigned char *scan;
-- 
2.34.1

[PATCH v4 6/7] char: xillybus: Add defensive sanity checks

[Eli Billauer]

Add validation checks for values derived from hardware or user input to
prevent incorrect behavior with malformed data.

Assisted-by: Deepseek:v4-pro Kimi:K2.6 ChatGPT:GPT-5.5 Claude:Sonnet-4.6
Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---

Notes:
    Changelog:
    =========
    
    Changes v3->v4:
     -- xillyusb.c: Use mutex_unlock() in response to sanity check failure
                    in fifo_init(), as guard() isn't used anymore on this
                    mutex.
     -- xillyusb.c and xillybus_core.c: Remove sanity check on data count on
                    read() and write() fops methods, as this check is already
                    done by the kernel's vfs_read() and vfs_write().
    
    Changes v2->v3:
     -- Add Assisted-by tag to description
    
    Changes v1->v2:
     -- xillybus_class.c: Assign @rc a value before goto in
                          xillybus_init_chrdev().
     -- xillybus_class.c: Improve check on @inode in xillybus_find_inode().
     -- xillybus_of.c:    Remove redundant dev_err(), as platform_get_irq()
                          outputs an error message if necessary.

 drivers/char/xillybus/xillybus_class.c | 18 +++++++++++++--
 drivers/char/xillybus/xillybus_class.h |  3 +++
 drivers/char/xillybus/xillybus_core.c  | 31 ++++++++++++++++++++++++--
 drivers/char/xillybus/xillybus_of.c    |  3 +++
 drivers/char/xillybus/xillyusb.c       | 25 ++++++++++++++++++++-
 5 files changed, 75 insertions(+), 5 deletions(-)

diff --git a/drivers/char/xillybus/xillybus_class.c b/drivers/char/xillybus/xillybus_class.c
index 5e8f03b77064..f7e0da233e2a 100644
--- a/drivers/char/xillybus/xillybus_class.c
+++ b/drivers/char/xillybus/xillybus_class.c
@@ -57,6 +57,9 @@ int xillybus_init_chrdev(struct device *dev,
 	size_t namelen;
 	struct xilly_unit *unit, *u;
 
+	if (num_nodes <= 0 || num_nodes > XILLYBUS_MAX_NODES || !idt || !prefix || !dev)
+		return -ENODEV;
+
 	unit = kzalloc_obj(*unit);
 
 	if (!unit)
@@ -68,6 +71,12 @@ int xillybus_init_chrdev(struct device *dev,
 		snprintf(unit->name, UNITNAMELEN, "%s", prefix);
 
 	for (i = 0; enumerate; i++) {
+		if (i > 99) {
+			dev_err(dev, "Failed to obtain unique unit name\n");
+			rc = -ENODEV;
+			goto fail_obtain;
+		}
+
 		snprintf(unit->name, UNITNAMELEN, "%s_%02d",
 			 prefix, i);
 
@@ -215,10 +224,15 @@ EXPORT_SYMBOL(xillybus_cleanup_chrdev);
 int xillybus_find_inode(struct inode *inode,
 			void **private_data, int *index)
 {
-	int minor = iminor(inode);
-	int major = imajor(inode);
+	int minor, major;
 	struct xilly_unit *unit = NULL, *iter;
 
+	if (!inode || !private_data || !index)
+		return -ENODEV;
+
+	minor = iminor(inode);
+	major = imajor(inode);
+
 	mutex_lock(&unit_mutex);
 
 	list_for_each_entry(iter, &unit_list, list_entry)
diff --git a/drivers/char/xillybus/xillybus_class.h b/drivers/char/xillybus/xillybus_class.h
index 5dbfdfc95c65..4dbed9adcaf8 100644
--- a/drivers/char/xillybus/xillybus_class.h
+++ b/drivers/char/xillybus/xillybus_class.h
@@ -8,6 +8,9 @@
 #ifndef __XILLYBUS_CLASS_H
 #define __XILLYBUS_CLASS_H
 
+#define XILLYBUS_MAX_NODES 1024
+#define XILLYBUS_MAX_IDT 1048576
+
 #include <linux/types.h>
 #include <linux/device.h>
 #include <linux/fs.h>
diff --git a/drivers/char/xillybus/xillybus_core.c b/drivers/char/xillybus/xillybus_core.c
index b264578b2572..6bc72d9dfb16 100644
--- a/drivers/char/xillybus/xillybus_core.c
+++ b/drivers/char/xillybus/xillybus_core.c
@@ -351,6 +351,12 @@ static int xilly_get_dma_buffers(struct xilly_endpoint *ep,
 	struct device *dev = ep->dev;
 	struct xilly_buffer *this_buffer = NULL; /* Init to silence warning */
 
+	if (bytebufsize == 0 || bytebufsize > 0x40000000) {
+		dev_err(ep->dev,
+			"Illegal buffer size requested in IDT. Aborting.\n");
+		return -ENODEV;
+	}
+
 	if (buffers) { /* Not the message buffer */
 		this_buffer = devm_kcalloc(dev, bufnum,
 					   sizeof(struct xilly_buffer),
@@ -623,6 +629,12 @@ static int xilly_scan_idt(struct xilly_endpoint *endpoint,
 		return -ENODEV;
 	}
 
+	if (count == 0 || count > XILLYBUS_MAX_NODES) {
+		dev_err(endpoint->dev,
+			"Unreasonable number of channels. Aborting.\n");
+		return -ENODEV;
+	}
+
 	idt_handle->entries = len >> 2;
 	endpoint->num_channels = count;
 
@@ -725,8 +737,18 @@ static ssize_t xillybus_read(struct file *filp, char __user *userbuf,
 			bufidx = channel->wr_host_buf_idx;
 			bufpos = channel->wr_host_buf_pos;
 			howmany = ((channel->wr_buffers[bufidx]->end_offset
-				    + 1) << channel->log2_element_size)
-				- bufpos;
+				    + 1) << channel->log2_element_size);
+
+			if (howmany > channel->wr_buf_size ||
+			    howmany < bufpos) {
+				dev_err(channel->endpoint->dev,
+					"Illegal buffer fill level from hardware\n");
+				channel->endpoint->fatal_error = 1;
+				spin_unlock_irqrestore(&channel->wr_spinlock, flags);
+				break;
+			}
+
+			howmany	-= bufpos;
 
 			/* Update wr_host_* to its post-operation state */
 			if (howmany > bytes_to_do) {
@@ -1902,6 +1924,11 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 		return -ENODEV;
 	}
 
+	if (endpoint->idtlen < 4 || endpoint->idtlen > XILLYBUS_MAX_IDT) {
+		dev_err(endpoint->dev, "Invalid IDT length. Aborting.\n");
+		return -ENODEV;
+	}
+
 	/* Enable DMA */
 	iowrite32((u32) (0x0002 | (endpoint->dma_using_dac & 0x0001)),
 		  endpoint->registers + fpga_dma_control_reg);
diff --git a/drivers/char/xillybus/xillybus_of.c b/drivers/char/xillybus/xillybus_of.c
index 46e1046abfca..44b0c754deb2 100644
--- a/drivers/char/xillybus/xillybus_of.c
+++ b/drivers/char/xillybus/xillybus_of.c
@@ -53,6 +53,9 @@ static int xilly_drv_probe(struct platform_device *op)
 
 	irq = platform_get_irq(op, 0);
 
+	if (irq < 0)
+		return irq;
+
 	rc = devm_request_irq(dev, irq, xillybus_isr, 0, xillyname, endpoint);
 
 	if (rc)
diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index e2270a64b659..5b6a15962885 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -396,6 +396,12 @@ static int fifo_init(struct xillyfifo *fifo,
 	fifo->size = fifo->bufnum * fifo->bufsize;
 	fifo->buf_order = buf_order;
 
+	if (!fifo->size || /* Unsigned integer overflow */
+	    fifo->size > 0x40000000) { /* Avoid signed int issues */
+		mutex_unlock(&fifo_buf_order_mutex);
+		return -ENOMEM; /* Reported as greed for memory */
+	}
+
 	fifo->mem = kmalloc_array(fifo->bufnum, sizeof(void *), GFP_KERNEL);
 
 	if (!fifo->mem) {
@@ -893,6 +899,7 @@ static int process_in_opcode(struct xillyusb_dev *xdev,
 	struct xillyusb_channel *chan;
 	struct device *dev = xdev->dev;
 	int chan_idx = chan_num >> 1;
+	struct xillyfifo *in_fifo;
 
 	if (chan_idx >= xdev->num_channels) {
 		dev_err(dev, "Received illegal channel ID %d from FPGA\n",
@@ -917,7 +924,10 @@ static int process_in_opcode(struct xillyusb_dev *xdev,
 		 */
 		smp_wmb();
 		WRITE_ONCE(chan->read_data_ok, 0);
-		wake_up_interruptible(&chan->in_fifo->waitq);
+
+		in_fifo = READ_ONCE(chan->in_fifo);
+		if (in_fifo)
+			wake_up_interruptible(&in_fifo->waitq);
 		break;
 
 	case OPCODE_REACHED_CHECKPOINT:
@@ -2077,6 +2087,13 @@ static int xillyusb_discovery(struct usb_interface *interface)
 	}
 
 	idt_len = READ_ONCE(idt_fifo.fill);
+
+	if (idt_len < 4 || idt_len > XILLYBUS_MAX_IDT) {
+		rc = -ENODEV;
+		dev_err(&interface->dev, "Invalid IDT length. Aborting.\n");
+		goto unfifo;
+	}
+
 	idt = kmalloc(idt_len, GFP_KERNEL);
 
 	if (!idt) {
@@ -2111,6 +2128,12 @@ static int xillyusb_discovery(struct usb_interface *interface)
 		goto unidt;
 	}
 
+	if (num_channels == 0 || num_channels > XILLYBUS_MAX_NODES) {
+		dev_err(&interface->dev, "Unreasonable number of channels. Aborting.\n");
+		rc = -ENODEV;
+		goto unidt;
+	}
+
 	rc = setup_channels(xdev, (void *)idt + 3, num_channels);
 
 	if (rc)
-- 
2.34.1

[PATCH v4 7/7] char: xillybus: Ignore and report unsolicited interrupts

[Eli Billauer]

During initialization, the hardware should issue interrupts only in
response to requests from the host. Ignore and log unexpected interrupts,
as these indicate misbehaving hardware, and return IRQ_NONE when the
interrupt appears to be spurious.

In the same spirit, in xilly_quiesce(), assign endpoint->num_channels = 0
before allowing the ISR, in order to expose whether the hardware
incorrectly sends messages related to data channels during shutdown.

Assisted-by: Deepseek:v4-pro Kimi:K2.6 ChatGPT:GPT-5.5 Claude:Sonnet-4.6
Assisted-by: Sashiko-0.2.5:gemini-3.1-pro-preview
Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---

Notes:
    Changelog:
    =========
    
    Changes v3->v4:
     -- Return IRQ_NONE if the interrupt is considered spurious, following
        Sashiko's remark + add attribution to Sashiko.
    
    Changes v2->v3:
     -- Add Assisted-by tag to description
    
    No change on v1->v2.

 drivers/char/xillybus/xillybus.h      |  3 ++
 drivers/char/xillybus/xillybus_core.c | 47 ++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/char/xillybus/xillybus.h b/drivers/char/xillybus/xillybus.h
index 51de7cbc579e..98c7ac4dd1f9 100644
--- a/drivers/char/xillybus/xillybus.h
+++ b/drivers/char/xillybus/xillybus.h
@@ -94,6 +94,9 @@ struct xilly_endpoint {
 	__iomem void *registers;
 	int fatal_error;
 
+	bool allow_isr;
+	spinlock_t allow_isr_lock;
+
 	struct mutex register_mutex;
 	wait_queue_head_t ep_wait;
 
diff --git a/drivers/char/xillybus/xillybus_core.c b/drivers/char/xillybus/xillybus_core.c
index 6bc72d9dfb16..037e3801d068 100644
--- a/drivers/char/xillybus/xillybus_core.c
+++ b/drivers/char/xillybus/xillybus_core.c
@@ -72,6 +72,8 @@ static struct workqueue_struct *xillybus_wq;
  *
  * rd_spinlock does the same with rd_*_buf_idx, rd_empty and end_offset.
  *
+ * allow_isr_lock protects allow_isr.
+ *
  * register_mutex is endpoint-specific, and is held when non-atomic
  * register operations are performed. wr_mutex and rd_mutex may be
  * held when register_mutex is taken, but none of the spinlocks. Note that
@@ -84,7 +86,8 @@ static struct workqueue_struct *xillybus_wq;
  * Only interruptible blocking is allowed on mutexes and wait queues.
  *
  * All in all, the locking order goes (with skips allowed, of course):
- * wr_mutex -> rd_mutex -> register_mutex -> wr_spinlock -> rd_spinlock
+ * wr_mutex -> rd_mutex -> register_mutex ->
+ * allow_isr_lock -> wr_spinlock -> rd_spinlock
  */
 
 static void malformed_message(struct xilly_endpoint *endpoint, u32 *buf)
@@ -119,6 +122,13 @@ irqreturn_t xillybus_isr(int irq, void *data)
 	unsigned int msg_channel, msg_bufno, msg_data, msg_dir;
 	struct xilly_channel *channel;
 
+	guard(spinlock)(&ep->allow_isr_lock);
+
+	if (!ep->allow_isr) {
+		dev_err_ratelimited(ep->dev, "Unexpected interrupt! Something is wrong with the hardware.\n");
+		return IRQ_NONE;
+	}
+
 	buf = ep->msgbuf_addr;
 	buf_size = ep->msg_buf_size/sizeof(u32);
 
@@ -137,6 +147,7 @@ irqreturn_t xillybus_isr(int irq, void *data)
 			if (++ep->failed_messages > 10) {
 				dev_err(ep->dev,
 					"Lost sync with interrupt messages. Stopping.\n");
+				return IRQ_NONE;
 			} else {
 				dma_sync_single_for_device(ep->dev,
 							   ep->msgbuf_dma_addr,
@@ -283,6 +294,19 @@ irqreturn_t xillybus_isr(int irq, void *data)
 }
 EXPORT_SYMBOL(xillybus_isr);
 
+/*
+ * xilly_allow_isr() is similar to enabling / disabling the interrupt,
+ * with the difference that if an interrupt is issued while ep->allow_isr
+ * is false, this is visible in the kernel log.
+ */
+
+static void xilly_allow_isr(struct xilly_endpoint *ep, bool newstate)
+{
+	guard(spinlock_irqsave)(&ep->allow_isr_lock);
+
+	ep->allow_isr = newstate;
+}
+
 /*
  * A few trivial memory management functions.
  * NOTE: These functions are used only on probe and remove, and therefore
@@ -651,6 +675,8 @@ static int xilly_obtain_idt(struct xilly_endpoint *endpoint)
 
 	channel->wr_sleepy = 1;
 
+	xilly_allow_isr(endpoint, true);
+
 	iowrite32(1 |
 		  (3 << 24), /* Opcode 3 for channel 0 = Send IDT */
 		  endpoint->registers + fpga_buf_ctrl_reg);
@@ -659,6 +685,8 @@ static int xilly_obtain_idt(struct xilly_endpoint *endpoint)
 					     (!channel->wr_sleepy),
 					     XILLY_TIMEOUT);
 
+	xilly_allow_isr(endpoint, false);
+
 	if (t <= 0) {
 		dev_err(endpoint->dev, "Failed to obtain IDT. Aborting.\n");
 
@@ -1837,6 +1865,9 @@ struct xilly_endpoint *xillybus_init_endpoint(struct device *dev)
 	endpoint->failed_messages = 0;
 	endpoint->fatal_error = 0;
 
+	endpoint->allow_isr = false;
+	spin_lock_init(&endpoint->allow_isr_lock);
+
 	init_waitqueue_head(&endpoint->ep_wait);
 	mutex_init(&endpoint->register_mutex);
 
@@ -1849,6 +1880,9 @@ static int xilly_quiesce(struct xilly_endpoint *endpoint)
 	long t;
 
 	endpoint->idtlen = -1;
+	endpoint->num_channels = 0;
+
+	xilly_allow_isr(endpoint, true);
 
 	iowrite32((u32) (endpoint->dma_using_dac & 0x0001),
 		  endpoint->registers + fpga_dma_control_reg);
@@ -1856,6 +1890,9 @@ static int xilly_quiesce(struct xilly_endpoint *endpoint)
 	t = wait_event_interruptible_timeout(endpoint->ep_wait,
 					     (endpoint->idtlen >= 0),
 					     XILLY_TIMEOUT);
+
+	xilly_allow_isr(endpoint, false);
+
 	if (t <= 0) {
 		dev_err(endpoint->dev,
 			"Failed to quiesce the device on exit.\n");
@@ -1909,6 +1946,8 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 
 	endpoint->idtlen = -1;
 
+	xilly_allow_isr(endpoint, true);
+
 	/*
 	 * Set DMA 32/64 bit mode, quiesce the device (?!) and get IDT
 	 * buffer size.
@@ -1919,6 +1958,9 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 	t = wait_event_interruptible_timeout(endpoint->ep_wait,
 					     (endpoint->idtlen >= 0),
 					     XILLY_TIMEOUT);
+
+	xilly_allow_isr(endpoint, false);
+
 	if (t <= 0) {
 		dev_err(endpoint->dev, "No response from FPGA. Aborting.\n");
 		return -ENODEV;
@@ -1945,6 +1987,7 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 	if (rc)
 		goto failed_idt;
 
+	/* xilly_obtain_idt() allows and then disallows the ISR */
 	rc = xilly_obtain_idt(endpoint);
 	if (rc)
 		goto failed_idt;
@@ -1963,6 +2006,8 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 	if (rc)
 		goto failed_idt;
 
+	xilly_allow_isr(endpoint, true);
+
 	rc = xillybus_init_chrdev(dev, &xillybus_fops,
 				  endpoint->owner, endpoint,
 				  idt_handle.names,
-- 
2.34.1

Re: [PATCH v4 0/7] char: xillybus: Harden driver and improve code quality

[Eli Billauer]

Please disregard this patch set. I will soon submit a v5, correcting yet 
another issue that Sashiko pointed out.

Regards,
    Eli